-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add n_iter_ attribute to estimators #79
base: main
Are you sure you want to change the base?
Conversation
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 for adding this @pentschev, this PR looks really good!
Minor comment: could you please add an n_iter_
attribute to the estimator docstrings
Also, as you mentioned, these changes will cause breakage in dask-ml
. Do you have an interest in updating the estimators over there too? Definitely no obligation though, just wanted to check
Yes, thanks for pointing that I forgot it.
Yes, I planned to do that once this was merged, I think there's no point in already opening an PR there before this gets reviewed here. |
Awesome, thank you @pentschev |
From my side, this is ready for another review/merge. |
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
Are we good to merge this? |
After this is merged, Dask-ML must be adapted to this change as well.
https://github.com/dask/dask-ml/blob/master/dask_ml/linear_model/glm.py#L187
Solves #77.