Add GRU classifier and GRU time series model#70
Add GRU classifier and GRU time series model#70typhoonzero merged 3 commits intosql-machine-learning:developfrom Derek-Wds:develop
Conversation
sneaxiy
left a comment
sneaxiy
left a comment
There was a problem hiding this comment.
Why not using WITH statements to choose LSTM or GRU model, instead of copying most of the model codes from LSTM-based models?
|
Yes, that is definitely a better solution. If this solution makes sense, we could even combine the RNN family models (RNN, LSTM, GRU) into one class. At the same time, we could give the user freedom to choose whether using the If this sounds good, I can update my PR to make an initial version of it. |
Sounds cool. |
|
Hi, I have just updated the codes for RNN classifier and RNN TS model, where I combine vanilla RNN, LSTM and GRU into one model respectively. Hope this is readable and more efficient. |
typhoonzero
left a comment
typhoonzero
left a comment
There was a problem hiding this comment.
LGTM generally. Please be aware that the model name LSTMBasedTimeSeriesModel is already used in SQLFlow's unit test: https://github.com/sql-machine-learning/sqlflow/blob/c58f0a4f6b08984c667d3ace1aa21d09aa6112de/cmd/sqlflowserver/e2e_mysql_test.go#L194 and the tutorial https://github.com/sql-machine-learning/sqlflow/blob/develop/doc/tutorial/energe_lstmbasedtimeseries.md. Can you pleas also update those two places after this PR was merged?
| :param n_features: number of features in every time window. | ||
| type n_features: int | ||
| type n_features: int. | ||
| :param model_type: Specific RNN model to be used. |
There was a problem hiding this comment.
Add a description about possible values, e.g. "rnn", "lstm" and "gru"?
sqlflow_models/rnnclassifier.py
Outdated
| :type stack_units: vector of ints. | ||
| :param n_classes: Target number of classes. | ||
| :type n_classes: int. | ||
| :param model_type: Specific RNN model to be used. |
|
Hi, I have just added one sentence for the description of the model type. Please take a look.
Yeah, sure! Once this PR is being merged, I will try my best to modify the unit tests and tutorials in two days. |
3 participants
Hi, I have added two GRU models in this PR 😄 . Since, the architectures of the two LSTM models used in this repo are reasonable, I just wrote the GRU models based on them with simple modifications. I have also provided the unit test codes, and all tests passed on my machine.
If there are any problems or additional things to do, please let me know. Hope it helps!
Derek