Add requires_positive_y estimator tag #14095
Conversation
| @@ -1519,6 +1519,9 @@ non_deterministic | |||
| requires_positive_data - unused for now | |||
| whether the estimator requires positive X. | |||
|
|
|||
| requires_positive_y | |||
| whether the estimator requires a positive y (only applicable for regression). | |||
There was a problem hiding this comment.
Maybe we should call the one above requires_positive_X to be consistent, not sure. In any case, I am not so keen on requires_positive_target if we try to reach naming consistency in the other direction.
There was a problem hiding this comment.
Why do you not like target? I'm fine either way.
There was a problem hiding this comment.
Wanna actually add the one above and do the same thing you did for y? The tag is not used in the estimator or tags yet, I think.
There was a problem hiding this comment.
Why do you not like target? I'm fine either way.
Mostly to be consistent with fit(X, y), but it's not too critical I agree.
Wanna actually add the one above and do the same thing you did for y? The tag is not used in the estimator or tags yet, I think.
Sure will make another PR.
|
|
||
| # doesn't error on actual estimator | ||
| LogisticRegression, | ||
| LogisticRegression(), |
There was a problem hiding this comment.
Previously this run on AdaBoostClassifier which took significant time (around 3 sec) for two checks. Since it looks like this only intends to check that initialized/non initialized estimators work, replaced it by a faster estimator.
AdaBoostClassifier is picked up by common tests anyway.
|
(The test failure in one of the jobs is unrelated). |
|
In case you are able to have a quick look (should be easy to review) @thomasjpfan @glemaitre. Already has a +1 ) |
|
I think I have addressed all comments. If there aren't new ones, could someone please merge this then, as this PR has a +2? Thanks! |
There was a problem hiding this comment.
I think we should just get some consensus on naming: requires_positive_y vs requires_positive_target. @rth prefers the former, I think I prefer the latter for consistency with requires_positive_data. Other opinions? Does it matter if tags are still experimental?
|
We have TLDR: +0.5 on |
|
OK, changed |
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
|
Thanks, @jnothman -- I added your suggestion! This should be good to merge then (CI is green)? |
This adds a
requires_positive_yestimator tag, for estimators that only work with a positive y for regression, such as Poisson regression in the GLM PR #9405.This tag only makes sense for regressors, but this is not enforced. Estimator tag validation in general could be done in some other PR.
The GLM PR distinguishes estimators, that work with strictly positive y as well as positive or zero. Here I only consider the strictly positive case, as I think for common tests that is enough.