API Deprecates copy_X in TheilSenRegressor#29105
API Deprecates copy_X in TheilSenRegressor#29105OmarManzoor merged 12 commits intoscikit-learn:mainfrom
copy_X in TheilSenRegressor#29105Conversation
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
There was a problem hiding this comment.
Thanks for the PR @adam2392. I left some comments, and I believe you also need to:
- Change the default value of
copy_Xto"deprecated". - In the
fitmethod, raise in the very beginning aFutureWarningifcopy_X != "deprecated"with a comprehensive message. You may refer to other deprecation warnings. - And yes I think you need to add a test. Normally the deprecated parameter will be explicitly set in some tests and then one can assert deprecation warnings for those tests. In this case I think
copy_Xis simply not tested anywhere, so you may want to add one yourself.
This part of the developers' docs might also be helpful.
Signed-off-by: Adam Li <[email protected]>
|
Ah nice! Thanks for pointing out that part of the documentation. Haven't seen it before. The last commit 7d23696 should address your comments. Thanks for the quick review! |
There was a problem hiding this comment.
Thanks for the quick update @adam2392! Just some nitpicks and minor mistakes left, otherwise LGTM:
-s Signed-off-by: Adam Li <[email protected]>
Charlie-XIAO
left a comment
Charlie-XIAO
left a comment
There was a problem hiding this comment.
Sorry @adam2392 for repeatedly asking for changes. It seems that I've got some pending comments that weren't successfully sent out in the previous review. They are very minor so I'm approving in advance. Thanks for the contribution!
Co-authored-by: Yao Xiao <[email protected]>
Co-authored-by: Yao Xiao <[email protected]>
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
Thanks for the PR @adam2392
Co-authored-by: Omar Salman <[email protected]>
Co-authored-by: Omar Salman <[email protected]>
OmarManzoor
left a comment
OmarManzoor
left a comment
There was a problem hiding this comment.
LGTM. Thanks @adam2392
3 participants
Reference Issues/PRs
Fixes: #29098
What does this implement/fix? Explain your changes.
Deprecates the
copy_Xparameter as it is not used.Any other comments?
I'm unsure of two things: