ENH Sets assume_finite in _non_negative_factorization #18581
Conversation
There was a problem hiding this comment.
I think I'd be +0 to introduce this parameter. IMHO a cleaner way would be for the function to call the estimator, not the other way around.
Some related discussion in #14897
|
(As a side note, the notion of "private parameters" has been in the back of my mind for a while. Basically a parameter that we use internally but that we don't expose to users. I'm not sure what it should look like, but I feel like this could be very useful to us. ) |
|
I have been thinking of doing stuff like: def non_negative_factorization(_check_input=False)where we use the underscore as a convention. The other option would be to have a private method: def non_negative_factorization(...):
X = check_array(X, ...)
_non_negative_factorization(X, ...)
class NMF:
def fit_transform(self, ...):
X = self._valdiate_data(X, ...)
... = _non_negative_factorization(X, ...)where
In this case |
|
Hm OK, I feel like having a private
Yeah, leading underscore + some sphinx magic so that these parameters docs aren't rendered might be a way. Pretty unconventional though :p |
|
I understand the reluctance to introduce new parameters for internal stuff, but |
|
I like the decorator approach used in #18727. That only concerns the finiteness check, though, and I wonder if any of the other checks are expensive. If so, we could add another global config or rename this one? |
|
I like it too ! |
|
That was my feeling as well, though checking is always good ;) |
|
Updated to use a context manager. |
Co-authored-by: Tom Dupré la Tour <tom.dupre-la-tour@m4x.org>
|
Two CI jobs failed when doing the git checkout. I triggered them again. |
|
Merged! |
Reference Issues/PRs
Continues #18557
CC @ogrisel @NicolasHug