Move stateful initialization to a pre_fit function#159
Merged
kiudee merged 2 commits intokiudee:masterfrom Sep 16, 2020
Merged
Conversation
Now that some pre_fit initialization is inherited, we can move the initialization there. This will satisfy the scikit-learn estimator requirements (__init__ should only store the parameters).
timokau
changed the title
kiudee
approved these changes
Sep 16, 2020
Owner
kiudee
left a comment
kiudee
left a comment
There was a problem hiding this comment.
Looks like a very clean solution. I like it.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Creates a
_pre_fitfunction to bridge the gap between__init__andfit: Everything which initializes some state, but does not actually depend on the data, belongs there. This separates concerns a bit and makes it possible to inherit those initializations.I thought about making this an implicit wrapper (a wrapper defined in
learnerthat inherits signature & docs and automatically prepends a call topre_fitto everyfitcall). This might be possible withwraptand a meta-class, but it would be a little complex. Since it would also be less explicit, it might be a bit confusing for people who are not very familiar with the code. So I just went with an explicit call to_pre_fitfor now.I could imagine to automate the calls to
initialize_optimizerandinitiialize_regularizerin the future, but again I'm not entirely certain its desirable because it would feel a little "magic".Motivation and Context
#157 (comment)
How Has This Been Tested?
Pre-commit & test suite.
Does this close/impact existing issues?
Impacts #94, #116, #146.
Types of changes
Checklist: