-
Notifications
You must be signed in to change notification settings - Fork 8
refactor: move optimization out of init and into fit #170
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
Conversation
nice progress. In general we want to support both UCs (multiple fits from the same starting point) and squential refinements (multiple fits starting the next fit from the previous one), so figuring out a structure that can support either based on some user input is best. |
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.
it looks good. I can merge it if you like. Just let me know. Some general questions
- why did you add the underscore after the attributes?
- This may be the moment we want to start writing tests so we can test these things. For example, a fairly easy test would be one that instantiates the class but doesn't run anything. What things do we want/need to be defined in the instantiated class? Write a test that instantiates the class and loads basic things and then tests that the class exists and has the right things in it.
Testing the fit()
will be a bigger lift.
For 1. , Per sklearn, attributes learned from user data should end with an underscore. For 2., That makes sense. I'll write a test for instantiating the class next. |
interesting. I like it! |
@sbillinge sklearn seems to use |
As part of the slow march towards
scikit-learn
compatibility, instantiating a model no longer immediately runs the optimization. This should make implementing something like a grid search a lot easier, because you can repeatedly callfit()
with different rho and eta instead of having to__init__
over and over.There are still a few things to address that make this not usable for the case I just described: for example, calling fit() repeatedly with different parameters would currently attempt to re-optimize the already found components, weights, and stretch, rather than reverting to the initial guesses to try again.
Then again, I'm not sure reverting to initial guesses is what sklearn does either. There are valid situations where you want to re-fit the existing fit. In fact, if they were decently correct, you might not mind starting from there at all. I'll admit I haven't looked into what the established way of handling this is, but the PR was getting large so I wanted to put it out for review. But deciding what to do here should probably be resolved before merging.