DOC: add transition guide & ease fit/predict kwarg compatibility #138

adriangb · 2020-11-27T23:34:44Z

Closes #124

codecov-io · 2020-11-28T02:54:13Z

Codecov Report

Merging #138 (03fc71c) into master (45da4ad) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #138   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files           5        5           
  Lines         614      623    +9     
=======================================
+ Hits          611      620    +9     
  Misses          3        3

Impacted Files	Coverage Δ
scikeras/utils/transformers.py	`100.00% <ø> (ø)`
scikeras/wrappers.py	`99.18% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45da4ad...2b97762. Read the comment docs.

README.md

adriangb · 2020-11-28T18:39:34Z

scikeras/wrappers.py

+        for k, v in kwargs.items():
+            warnings.warn(
+                "``kwargs`` will be removed in a future release of SciKeras."
+                f"Instead, set fit arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"
+            )
+        self.set_params(
+            **{
+                (k if k.startswith("fit__") else "fit__" + k): v
+                for k, v in kwargs.items()
+            }
+        )


@stsievert what are your thoughts on these implementations? The main con I see is that subsequent calls will re-use the previous parameters. That is:

est.fit(..., batch_size=32) ... est.fit(...) # implicitly uses batch_size=32 with no warnings to the user

👎 on this implementation. As a user, I would expect the params to behave the same unless I change them. Let the user do that, and issue an appropriate warning (like is being done).

PEP 20: "Explicit is better than implicit."

Which part of the implementation are you 👎 on? This is basically what you proposed, plus the prefixing of fit__ and predict__. Adding this prefix makes it behave more like the old implement, in the sense that passing a kwarg to fit won't "set" it for predict and visa versa.

Overall, I think there are 3 options:

We don't support **kwargs at all and force users to make the change.

This implementation.

This implementation + we "unset"/"revert" the kwargs at the end. I've tested this to work, but it is extra code and complexity. That said, this is the only one that is fully backwards compatible and transparent to users.

Edit: I implemented (3) above, but the diff needs some cleanup. I think we should pick between (1) and (3).

wrappers.py diff cleaned up. (3) is looking pretty good now.

stsievert · 2020-11-29T18:53:08Z

scikeras/wrappers.py

+            (k if k.startswith("fit__") else "fit__" + k): v for k, v in kwargs.items()
+        }
+        existing_kwargs = {k: v for k, v in self.get_params().items() if k in kwargs}
+        self.set_params(**kwargs)


👎

I'm still not seeing the need to set parameters. It looks like set_params is a vehicle to get these kwargs to self._fit or self._fit_keras_model. Why can't kwargs be passed to those functions and this processing be done there?

You were the one who originally suggested this approach in #124 (comment)

We could pass **kwargs around. Perhaps at this level of complexity/LOC that is a better approach. I'll try implementing it that way instead and see how it turns out.

I was able to implement this passing **kwargs around directly. It is indeed simpler and cleaner.

scikeras/wrappers.py

…cikeras into docs/transition-guide

scikeras/wrappers.py

Co-authored-by: Scott Sievert <[email protected]>

scikeras/wrappers.py

…cikeras into docs/transition-guide

adriangb · 2020-12-01T23:59:29Z

@stsievert are we ready to merge this? In light of dask/dask-ml#764 (comment) I'd like to cut a patch release, and I think it would be good if this is in it so that it can be a decent long term release that Keras users can switch over into.

stsievert · 2020-12-02T00:25:37Z

README.md

+pass your loss function to the constructor:
+
+```python
+clf = KerasClassifier(loss="categorical_crossentropy")  # or loss=CategoricalCrossentropy(), etc.


Suggested change

clf = KerasClassifier(loss="categorical_crossentropy") # or loss=CategoricalCrossentropy(), etc.

clf = KerasClassifier(loss="categorical_crossentropy")

README.md

stsievert · 2020-12-02T00:27:29Z

README.md

+Or to declare separate values for `fit` and `predict`:
+
+```python
+clf = KerasClassifier(fit__batch_size=32, predict__batch_size=10000)


Tangential to this PR, I think these would be good defaults.

stsievert · 2020-12-02T00:28:07Z

README.md

+```diff
+- def get_model(my_param=123):
+ def get_model(my_param):  # You can optionally remove the default here
+    ...


Suggested change

...

...

return model

stsievert · 2020-12-02T00:28:43Z

README.md

+ def get_model(my_param):  # You can optionally remove the default here
+    ...
+- clf = KerasClassifier(get_model)
+ clf = KerasClassifier(get_model, my_param=123)


Suggested change

+ clf = KerasClassifier(get_model, my_param=123)

+ clf = KerasClassifier(get_model, my_param=123) # option 1

+ clf = KerasClassifier(get_model, model__my_param=123) # option 2

stsievert · 2020-12-02T00:35:23Z

docs/source/index.rst

+
+   clf = KerasClassifier(fit__batch_size=32, predict__batch_size=10000)
+
+Renaming of ``build_fn`` to ``model``


Are these warnings in index.rst necessary? I'm having a hard time seeing the reason to feature the following on the front page of the documentation:

Removal of keyword args

Renaming build_fn to model

Model introspection

All of those issue warnings with tips on how to resolve. Why are they on the front page of the documentation? When I write docs, the front page always describes why a user should use my software, not issues they might face.

I might dedicate a separate page to potential hotspots when transitioning from TF to SciKeras.

Yeah a separate page should work and sounds good. Thanks.

stsievert · 2020-12-02T00:39:16Z

scikeras/wrappers.py

@@ -463,6 +464,8 @@ def _fit_keras_model(
            Number of epochs for which the model will be trained.
        initial_epoch : int
            Epoch at which to begin training.
+        **kwargs : Dict[str, Any]
+            Extra arguments to route to ``Model.fit``.


What arguments can these be? I would include a link to the Keras API docs here. https://keras.io/api/models/model_training_apis/

The current docs are at https://www.tensorflow.org/api_docs/python/tf/keras/Model?version=nightly#fit but yeah, I'll include that.

stsievert · 2020-12-02T00:40:02Z

scikeras/wrappers.py

+        for k, v in kwargs.items():
+            warnings.warn(
+                "``**kwargs`` has been deprecated in SciKeras 0.2.1 and support will be removed be 1.0.0."
+                f" Instead, set fit arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"


Suggested change

f" Instead, set fit arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"

f" Instead, set fit arguments at initialization (e.g, ``BaseWrapper({k}={v})`` "

f"or ``BaseWrapper(fit__{k}={v})``)"

stsievert · 2020-12-02T00:41:59Z

scikeras/wrappers.py

+        for k, v in kwargs.items():
+            warnings.warn(
+                "``**kwargs`` has been deprecated in SciKeras 0.2.1 and support will be removed be 1.0.0."
+                f" Instead, set predict arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"


Suggested change

f" Instead, set predict arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"

f" Instead, set predict arguments at initialization (i.e., ``BaseWrapper({k}={v})`` "

f"or ``BaseWrapper(predict__{k}={v})``)"

stsievert · 2020-12-02T00:43:06Z

scikeras/wrappers.py

+        for k, v in kwargs.items():
+            warnings.warn(
+                "``**kwargs`` has been deprecated in SciKeras 0.2.1 and support will be removed be 1.0.0."
+                f" Instead, set predict_proba arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"


Suggested change

f" Instead, set predict_proba arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"

f" Instead, set predict_proba arguments at initialization (i.e, ``BaseWrapper({k}={v})`` "

f"or ``BaseWrapper(predict__{k}={v})``"

Does that behavior work?

No, for the purposes of parameter routing predict_proba uses predict__ since they both call predict__ in Keras. We could alias them if you think that might make things easier. It might also be good to have an error or at least a warning for unknown prefixes. All of this would have to be a separate PR.

I've edited the suggested change to use the predict__ prefix.

adriangb · 2020-12-05T20:31:33Z

@stsievert I incorporated your suggestions manually instead of committing them because I moved all of this content to a migrations.rst page instead of having it in README.md and index.rst. Otherwise I think I largely incorporated it all of the suggestions.

adriangb added 3 commits November 27, 2020 17:34

DOC: add transition guide & easy fit/predict kwarg compatibility

e211462

Add note on default arguments

c784b66

fix code block

76120e5

stsievert reviewed Nov 28, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

PR feedback

2078b45

adriangb commented Nov 28, 2020

View reviewed changes

adriangb added 7 commits November 28, 2020 18:44

Set/reset kwargs

4cbd327

Duh forgot to add file

43a0ecf

Fix tests

af6c8fb

add **extra_kwargs to predict_proba test

835cfbc

Merge branch 'master' into docs/transition-guide

11e9f02

Fix merge

ee7b6c6

Fix arg swap

bb3e5f1

adriangb changed the title ~~DOC: add transition guide & easy fit/predict kwarg compatibility~~ DOC: add transition guide & ease fit/predict kwarg compatibility Nov 29, 2020

Merge branch 'master' into docs/transition-guide

661b242

stsievert reviewed Nov 29, 2020

View reviewed changes

adriangb added 2 commits November 29, 2020 13:35

Pass **kwargs directly

da89dc2

Merge branch 'docs/transition-guide' of https://github.com/adriangb/s…

91ab902

…cikeras into docs/transition-guide

stsievert reviewed Nov 30, 2020

View reviewed changes

scikeras/wrappers.py Outdated Show resolved Hide resolved

scikeras/wrappers.py Outdated Show resolved Hide resolved

scikeras/wrappers.py Outdated Show resolved Hide resolved

adriangb and others added 3 commits November 29, 2020 21:46

Update scikeras/wrappers.py

2321cd2

Co-authored-by: Scott Sievert <[email protected]>

Update scikeras/wrappers.py

0712eec

Co-authored-by: Scott Sievert <[email protected]>

PR suggestions

62adfe1

adriangb mentioned this pull request Dec 1, 2020

Cannot pass keyword parameters to build function through wrapper #144

Closed

adriangb added 6 commits November 30, 2020 21:36

Edit warning as per PR conversation.

2e66e4c

punctuation

68a199c

add deprecation version

b864123

fix test match string

a51f2d8

formatting

1c087d8

Merge branch 'master' into docs/transition-guide

bd2bef1

Merge branch 'master' into docs/transition-guide

11faff3

stsievert reviewed Dec 1, 2020

View reviewed changes

scikeras/wrappers.py Outdated Show resolved Hide resolved

scikeras/wrappers.py Outdated Show resolved Hide resolved

scikeras/wrappers.py Outdated Show resolved Hide resolved

adriangb added 2 commits December 1, 2020 10:33

PR feedback

7dd1cd4

Merge branch 'docs/transition-guide' of https://github.com/adriangb/s…

ebccfc0

…cikeras into docs/transition-guide

stsievert reviewed Dec 2, 2020

View reviewed changes

adriangb added 2 commits December 5, 2020 14:26

Create migration page, PR feedback

03fc71c

Add migrations page

2b97762

adriangb added 2 commits December 5, 2020 14:45

Update README.md

ef4d57d

Update README.md

d1362e4

adriangb merged commit 290c220 into master Dec 5, 2020

adriangb deleted the docs/transition-guide branch December 5, 2020 20:46

adriangb mentioned this pull request Jul 10, 2021

Can't pickle trained model with callback to TensorBoard #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: add transition guide & ease fit/predict kwarg compatibility #138

DOC: add transition guide & ease fit/predict kwarg compatibility #138

adriangb commented Nov 27, 2020 •

edited

Loading

codecov-io commented Nov 28, 2020 •

edited

Loading

adriangb Nov 28, 2020

stsievert Nov 28, 2020

adriangb Nov 28, 2020 •

edited

Loading

adriangb Nov 29, 2020

stsievert Nov 29, 2020

adriangb Nov 29, 2020

adriangb Nov 29, 2020

adriangb commented Dec 1, 2020 •

edited

Loading

stsievert Dec 2, 2020

stsievert Dec 2, 2020

stsievert Dec 2, 2020

stsievert Dec 2, 2020

stsievert Dec 2, 2020

adriangb Dec 2, 2020

stsievert Dec 2, 2020

adriangb Dec 2, 2020

stsievert Dec 2, 2020

stsievert Dec 2, 2020

stsievert Dec 2, 2020 •

edited

Loading

adriangb Dec 2, 2020

stsievert Dec 2, 2020

adriangb commented Dec 5, 2020

	clf = KerasClassifier(loss="categorical_crossentropy") # or loss=CategoricalCrossentropy(), etc.
	clf = KerasClassifier(loss="categorical_crossentropy")

	+ clf = KerasClassifier(get_model, my_param=123)
	+ clf = KerasClassifier(get_model, my_param=123) # option 1
	+ clf = KerasClassifier(get_model, model__my_param=123) # option 2


		clf = KerasClassifier(fit__batch_size=32, predict__batch_size=10000)

		Renaming of ``build_fn`` to ``model``

	f" Instead, set fit arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"
	f" Instead, set fit arguments at initialization (e.g, ``BaseWrapper({k}={v})`` "
	f"or ``BaseWrapper(fit__{k}={v})``)"

	f" Instead, set predict arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"
	f" Instead, set predict arguments at initialization (i.e., ``BaseWrapper({k}={v})`` "
	f"or ``BaseWrapper(predict__{k}={v})``)"

	f" Instead, set predict_proba arguments at initialization (i.e., ``BaseWrapper({k}={v})``)"
	f" Instead, set predict_proba arguments at initialization (i.e, ``BaseWrapper({k}={v})`` "
	f"or ``BaseWrapper(predict__{k}={v})``"

DOC: add transition guide & ease fit/predict kwarg compatibility #138

DOC: add transition guide & ease fit/predict kwarg compatibility #138

Conversation

adriangb commented Nov 27, 2020 • edited Loading

codecov-io commented Nov 28, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsievert Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Dec 5, 2020

adriangb commented Nov 27, 2020 •

edited

Loading

codecov-io commented Nov 28, 2020 •

edited

Loading

adriangb Nov 28, 2020 •

edited

Loading

adriangb commented Dec 1, 2020 •

edited

Loading

stsievert Dec 2, 2020 •

edited

Loading