[MRG] Use tag requires_positive_X for NMF + ComplementNB #14680
Conversation
|
Shall we have a |
|
I had been starting a version of this locally but I just saw this PR (see #14685) Also, maybe it could be a good occasion to update the statements |
|
Also |
| estimator = clone(estimator_orig) | ||
| assert_raises_regex(ValueError, "Negative values in data passed to", | ||
| estimator.fit, X, y) | ||
|
|
There was a problem hiding this comment.
a redundant link (flake8 error which can't be detected in PR)
There was a problem hiding this comment.
sorry I don't get what you mean here. My editor does not complain at these lines (but elsewhere in the file)
There was a problem hiding this comment.
there're 3 lines between this functions and the next function.
E303 too many blank lines (3)
There was a problem hiding this comment.
my bad... that's what happens when you have so many flake8 errors in the file that you stop paying attention to them :(
| X = np.array([[-1., 1], [-1., 1]]) | ||
| y = np.array([1, 2]) | ||
| estimator = clone(estimator_orig) | ||
| assert_raises_regex(ValueError, "Negative values in data passed to", |
There was a problem hiding this comment.
We've decided to deprecate assert_raise_regex, see #14216
There was a problem hiding this comment.
I would need to import pytest in a non-test file. Do we have such a context manager in the testing module?
There was a problem hiding this comment.
I don't know, let's leave it as it is.
|
we should also do the inverse. Right now many tests use positive data, they should use this tag instead. |
|
but the current design that would mean passing the tags to the check
functions or getting the tags in each of them.
… |
|
We get the tags in most of them already, why would that be an issue? We check if they are multi-output only in each check I think. |
|
@wdevazelhes yes, ideally that'll replace the existing helpers on X. |
Done, in #14705 |
I need the tag
requires_positive_Xfor #13246 following the suggestion of @rth