Skip to content
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

ENH: Support routed parameters to callbacks #233

Merged
merged 38 commits into from
Jun 18, 2021
Merged

ENH: Support routed parameters to callbacks #233

merged 38 commits into from
Jun 18, 2021

Conversation

adriangb
Copy link
Owner

@adriangb adriangb commented Jun 1, 2021

Closes #232

TODO:

  • (maybe) support routing without the fit__ prefix.

@adriangb adriangb mentioned this pull request Jun 1, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2021

Codecov Report

Merging #233 (c88d277) into master (164505c) will decrease coverage by 0.81%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
- Coverage   99.71%   98.90%   -0.82%     
==========================================
  Files           6        6              
  Lines         693      728      +35     
==========================================
+ Hits          691      720      +29     
- Misses          2        8       +6     
Impacted Files Coverage Δ
scikeras/wrappers.py 98.00% <81.81%> (-1.46%) ⬇️
scikeras/_utils.py 100.00% <100.00%> (ø)

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 164505c...c88d277. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jun 1, 2021

📝 Docs preview for commit c88d277 at: https://www.adriangb.com/scikeras/refs/pull/233/merge/

tests/test_callbacks.py Outdated Show resolved Hide resolved
tests/test_callbacks.py Outdated Show resolved Hide resolved
@stsievert
Copy link
Collaborator

I am ambivalent on these changes being merged. The ugly syntax reduces my motivation. Is fit__callbacks=LearningRateScheduler possible?

@adriangb
Copy link
Owner Author

adriangb commented Jun 2, 2021

I am ambivalent on these changes being merged. The ugly syntax reduces my motivation. Is fit__callbacks=LearningRateScheduler possible?

Thank you for the quick feedback! Is there anything else, aside from the ugly syntax, that makes you doubtful? Does it seem non-useful or anything?

I agree, it's not the prettiest. I think your proposal should work (it may work as-is, if not, I'm fairly certain it can be made to work).

@adriangb
Copy link
Owner Author

adriangb commented Jun 2, 2021

I was able to incorporate your feedback, and also support with/without the fit__ prefix.

@adriangb adriangb marked this pull request as ready for review June 3, 2021 15:57
tests/test_callbacks.py Outdated Show resolved Hide resolved
@stsievert
Copy link
Collaborator

Is it common to have only one callback? It might be worth adding support for not passing a list/dict to callbacks then. That'd enable this syntax:

KerasEstimator(...
    callbacks=keras.callbacks.LearningRateScheduler,
    callbacks__0=Schedule,
    callbacks__0__coef=0.2,
)

I think that's a lot cleaner. I suppose this could also be enabled by having a callback keyword arg instead of callbacks.

The ugly syntax reduces my motivation. Is fit__callbacks=LearningRateScheduler possible?

I misspoke; I meant to say "is fit__callbacks=LearningRateScheduler(Scheduler(coef=0.2)) possible? I think the docs should use that example if so.

also support with/without the fit__ prefix.

In Keras parlance, is "callback" synonymous with "fit callback"? Or, in SciKeras parlance, could I have score__callbacks and predict__callbacks in addition to fit__callbacks?

@adriangb
Copy link
Owner Author

adriangb commented Jun 3, 2021

Is it common to have only one callback?

I think it's fairly common.

I suppose this could also be enabled by having a callback keyword arg instead of callbacks

Keras does not have a callback arg, only a callbacks arg; I think we should follow that.
As currently implemented here, all of the following will be supported:

# class list syntax
KerasClassifier(
    callbacks=[tf.keras.callbacks.EarlyStopping]
    callbacks__0__monitor="loss",
)
# class dict syntax
KerasClassifier(
    callbacks={"es": tf.keras.callbacks.EarlyStopping}
    callbacks__es__monitor="loss",
)
# single class syntax
KerasClassifier(
    callbacks=tf.keras.callbacks.EarlyStopping
    callbacks__monitor="loss",
)
# object list syntax
KerasClassifier(
    callbacks=[tf.keras.callbacks.EarlyStopping(monitor="loss")]
)
# object dict syntax
KerasClassifier(
    callbacks={"es": tf.keras.callbacks.EarlyStopping(monitor="loss")}
)
# single object syntax
KerasClassifier(
    callbacks=tf.keras.callbacks.EarlyStopping(monitor="loss")
)

Is fit__callbacks=LearningRateScheduler(Scheduler(coef=0.2)) possible? I think the docs should use that example if so.

As per above, it will be possible.

In Keras parlance, is "callback" synonymous with "fit callback"? Or, in SciKeras parlance, could I have score__callbacks and predict__callbacks in addition to fit__callbacks?

This is actually a really good point. In Keras, you can have callbacks for both fit and predict (as kwargs).
Also, because of how Keras' event-based callback API works, a single callback can be "dispatched" for both fit and predict.

We can support this as well, using fit__callbacks and predict__callbacks with the same semantics as batch_size and others:

  • Just callbacks passes them to both fit and predict.
  • fit__callbacks passes them to only callbacks (in addition to any callbacks defined by plain callbacks).
  • predict__callbacks passes them to only callbacks (in addition to any callbacks defined by plain callbacks).

And also:

  • Calling fit or initialize re-initializes all callbacks, including predict__callbacks.
  • Calling partial_fit does not re-initialize any callbacks.

docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
adriangb and others added 2 commits June 4, 2021 12:23
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
docs/source/advanced.rst Outdated Show resolved Hide resolved
@adriangb
Copy link
Owner Author

The nightly failures are the same in master, so I'm going to merge this regardless.

@adriangb adriangb merged commit 7c072fb into master Jun 18, 2021
@adriangb adriangb deleted the callbacks branch June 18, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of callbacks__
3 participants