-
Notifications
You must be signed in to change notification settings - Fork 229
SCML: Add warm_start parameter #345
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
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 workflow_dispatch but currently it is only set up with push and pull requests to master. So it would not be possible rn
(see https://docs.github.com/en/actions/managing-workflow-runs-and-deployments/managing-workflow-runs/manually-running-a-workflow)
This just needs a rebase to trigger the CI.
7329927 to
c732507
Compare
c732507 to
f591e37
Compare
CI triggered
@maxi-marufo @terrytangyuan well looks like checks have been cancelled and/or failed?
I think some failed which triggered cancellation of the rest.
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_startparameter instead of a newpartial_fitmethod 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.