-
-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Keras documentation #832
base: main
Are you sure you want to change the base?
Conversation
- Pass `loss` to the constructor as documented in https://www.adriangb.com/scikeras/stable/migration.html - Update package versions - Update syntax to use SciKeras compiled models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tests are failing because TF/Keras isn't installed on this branch. Could you comment out this line to make sure tests pass?
Line 53 in 0ea276d
condition: eq(variables['Build.SourceBranch'], 'refs/heads/main') |
The example on this doc page is tested in tests/model_selection/test_keras.py
. Can you update that test too?
$ pip install tensorflow>=2.3.0 | ||
$ pip install scikeras>=0.1.8 | ||
$ pip install tensorflow>=2.4.0 | ||
$ pip install scikeras>=0.3.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required? Dask-ML will still work with the removed versions, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things should work generally, but some of the syntax in this tutorial may not. I think we should either update the versions, or remove them altogether (since not specifying a version usually gets you the latest version anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dask-ML will not work with SciKeras v0.1.7. I think that version didn't have serialization (?).
We should make a note about the versioning. "The example below uses X. The usage with lower versions may be different than this example."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't there also some issue about serialization of stateful optimizers like Adam?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a note along the lines of #832 (comment)
Wasn't there also some issue about serialization of stateful optimizers like Adam?
Yeah, we fixed that in v0.3.0
, which is another good reason to bump the "recommended" version numbers in these docs, although I don't think we want to mention that here right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we fixed [serialization] in v0.3.0
That's a really good reason to require SciKeras v0.3.0.
@@ -36,24 +36,18 @@ normal way to create a `Keras Sequential model`_ | |||
from tensorflow.keras.layers import Dense | |||
from tensorflow.keras.models import Sequential | |||
|
|||
def build_model(lr=0.01, momentum=0.9): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The tests in e73daa9 were failing within TF (failure line). |
loss
to the constructor as documented in https://www.adriangb.com/scikeras/stable/migration.html