API Deprecates copy_X in TheilSenRegressor
#29105
Conversation
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
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 <adam2392@gmail.com>
|
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! |
Signed-off-by: Adam Li <adam2392@gmail.com>
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 <adam2392@gmail.com>
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 <108576690+Charlie-XIAO@users.noreply.github.com>
Co-authored-by: Yao Xiao <108576690+Charlie-XIAO@users.noreply.github.com>
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
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: