-
Notifications
You must be signed in to change notification settings - Fork 232
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
SCML: Add warm_start parameter #345
base: master
Are you sure you want to change the base?
SCML: Add warm_start parameter #345
Conversation
This change looks fine to me, though I'm not sure when this warm-start option is useful in practice. Sorry for the extreme delay in reviewing! @grudloff want to take a look? |
LGTM! |
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.
Thanks @maxi-marufo !
Maybe it is possible to design a better test, something in the spirit https://github.com/scikit-learn/scikit-learn/blob/80598905e517759b4696c74ecc35c6e2eb508cff/sklearn/linear_model/tests/test_sgd.py#L274
where the idea is to check that calling warm_start=True
is equivalent to manually setting the parameters?
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.
LGTM @maxi-marufo, but I would like to rerun the CIs, not sure how to do this - @perimosocordiae @terrytangyuan do you know?
One simple way sould be for you @maxi-marufo to update your branch to the current state of master and push an empty commit
@bellet For a manual run you need to have a trigger on |
This just needs a rebase to trigger the CI. |
Because SCML optimization procedure is based on stochastic subgradient descent, we can save the weights after fitting the model, and use them in a following fit call (with a different set of triplets). The decision to use the
warm_start
parameter instead of a newpartial_fit
method is because partial_fit in scikit-learn will only fit 1 epoch, whereas fit will fit for multiple epochs (until the loss converges or max_iter is reached), which is the case also for SCML.