[ModelZoo] unify with elastic dl model zoo #25
Conversation
| def loss(): | ||
| """Default loss function. Used in model.compile.""" | ||
| return 'sparse_categorical_crossentropy' |
There was a problem hiding this comment.
I am not sure if string representation would work in ElasticDL yet.
There was a problem hiding this comment.
Will add unit tests on models repo when merging ElasticDL model_zoo/ dir.
There was a problem hiding this comment.
Do you mean we'll test this later? I am quite sure this won't work in ElasticDL yet.
There was a problem hiding this comment.
Yep. Will fix this later in another PR.
typhoonzero
changed the title
| def loss(): | ||
| """Default loss function. Used in model.compile.""" | ||
| return 'sparse_categorical_crossentropy' |
There was a problem hiding this comment.
Do you mean we'll test this later? I am quite sure this won't work in ElasticDL yet.
| @@ -10,6 +12,7 @@ def __init__(self, feature_columns, stack_units=[32], hidden_size=64, n_classes= | |||
| :param n_classes: Target number of classes. | |||
| :type n_classes: int. | |||
| """ | |||
| global _loss | |||
There was a problem hiding this comment.
Why are these global keywords needed?
There was a problem hiding this comment.
Please, check out the above comment:
_train_lr may be configured by SQL statements when initializing the model instance. e.g.
SELECT * FROM mytrain_table TO TRAIN sqlflow_models.DNNClassifier WITH train_lr=0.0001 ...
In the above SQL statement, train_lr will be passed to DeepEmbeddingClusterModel to configure the learning rate.
The previous change #24 is not correct comparing to https://github.com/sql-machine-learning/elasticdl/blob/develop/model_zoo/cifar10_subclass/cifar10_subclass.py.
This PR changes how the customed model will be used in SQLFlow:
optimizer,lossfunction moved to module function, not a member of the model class to be the same as ElasticDL. And, if we putoptimizer,lossfunction inside the model class, it will cause keras model prediction error: https://travis-ci.com/sql-machine-learning/sqlflow/jobs/254846188