ENH Add "ensure_non_negative" option to check_array

[TamaraAtanasoska]

Reference Issues/PRs

Fixes #29508.

What does this implement/fix? Explain your changes.

Adding an option to check if an array has only_non_negative values to the sklearn.utils.validation.check_array function, that contains the sklearn.utils.validation.check_non_negative functionality.

While in the initial issue I proposed ensure_positive as a name for the option, I found only_non_negative with exactly the same function in sklearn.utils.validation._check_sample_weight(here) and I used the same for consistency.

This addition will prevent the need to use sklearn.utils.validation.check_non_negative after sklearn.utils.validation.check_array in the use case of needing only non-negative values in an array. I am keeping the proposed changes minimal for easier review, but I am happy to also change all occurrences in the scikit-learn code where this pattern exists as an added commit.

Any other comments?

[github-actions]

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 702f196. Link to the linter CI: here}

[jeremiedbb]

Thanks for the PR @TamaraAtanasoska. I'd like to keep the ensure_xxx naming pattern for consistency in check_array. Since _check_sample_weight is private, I'd rather change the name there if we really want to for both to have the same name.

It's possible though that ensure_positive may be confusing w.r.t whether or not zero is allowed. So ensure_non_negative is a valid option too.

[TamaraAtanasoska]

Thanks for the PR @TamaraAtanasoska. I'd like to keep the ensure_xxx naming pattern for consistency in check_array. Since _check_sample_weight is private, I'd rather change the name there if we really want to for both to have the same name.

It's possible though that ensure_positive may be confusing w.r.t whether or not zero is allowed. So ensure_non_negative is a valid option too.

Agreed, changed it everywhere to ensure_non_negative in e18e5f3.

[glemaitre]

We need to add an entry in the changelog located in doc/whats_new/v1.6.rst since it touch the public API of check_array. We don't need to document anything about _check_sample_weight there.

It is a bit weird that we introduce the option but that we don't use it.

@jeremiedbb do you think that we can use this feature directly in NMF because if I recall well this is one requirement of the transformer. I assume that we are currently calling the check_non_negative function directly in the code.

I don't know if there is something similar with transformer that expect counts as input in the feature extraction for text (like TfidfTransformer).

[glemaitre]

We should probably have a versionchanged to mention that we rename the parameter. @jeremiedbb I'm scared to break code but this should be quite limited and easy to handle for downstream package.

[TamaraAtanasoska]

doc/whats_new/v1.6.rst

Thanks for the review @glemaitre! I am looking into adding to the changelog now. Is this an enhancement or a feature? I will also add it in the places where check_non_negative follows check_array.

[glemaitre]

Is this an enhancement or a feature?

I would say an enhancement.

[TamaraAtanasoska]

@glemaitre @jeremiedbb a few notes on a6cfa3a:

• I added a short changelog entry. Let me know if it needs to be changed in any way, first time 😊
• I had to move the check at the very bottom of check_array, which mirrors its usage until now. Otherwise the input array is not "ready" and some changes will need to be made to check_non_negative.
• I added two example usages in the existing code. What comes to light when looking at the changes required for them is that there is a loss of information that previous was there, packed in whom which is now a generic check_array as source of input. In what cases is this information essential?

[glemaitre]

I added two example usages in the existing code. What comes to light when looking at the changes required for them is that there is a loss of information that previous was there, packed in whom which is now a generic check_array as source of input. In what cases is this information essential?

I think that we can alleviate this issue by using both input_name and estimator of the check_array: estimator will help at getting the name of the estimator while input_name will help at indicating which data container contains negative values. We could adapt a bit the error message of check_non_negative to have a meaningful message, or craft a good whom message from these two parameters.

[glemaitre]

As stated in another comment, if input_name and estimator are not the default, I think that we can make a crafted message better than "check_array".

[glemaitre]

I think that we can also modify NMF. Right now the check_non_negative is called in _fit_transform method. However, we could remove it and use the ensure_non_negative of check_array call in fit_transform and transform

[TamaraAtanasoska]

@glemaitre I tired to address all the comments above in 890b480. Not sure about adding a generic "estimator" in this line. Would it be better if it was empty maybe?

[glemaitre]

Would it be better if it was empty maybe?

I would probably not have a generic name and not mentioned it (so probably the empty yes). It would translate to something like:

    if ensure_non_negative:
        whom = input_name
        if estimator_name:
            whom += f" in {estimator_name}"
        check_non_negative(array, whom=whom)

[TamaraAtanasoska]

whom = input_name
        if estimator_name:
            whom += f" in {estimator_name}"

I love this suggestion, just pushed a commit with it. It preserves the whom that makes the usage of it obvious and adds a more natural end of sentence without an estimator name.

[jeremiedbb]

Suggested change

	Make sure the array has only non-negative values. An array that contains
	non-negative values will raise a ValueError.
	Make sure the array has only non-negative values. If True, an array that contains
	negative values will raise a ValueError.

[jeremiedbb]

I would put that just before the previous if force_writeable block to be able to fail fast, before a pontential copy.

[jeremiedbb]

why didn't you keep "precomputed distance matrix" ?

[glemaitre]

Agreed, graph is an internal variable and people might get confused by the error message then.

[TamaraAtanasoska]

thanks @jeremiedbb , makes sense! I didn't notice there is more relevant naming there.

[jeremiedbb]

@glemaitre, should we add it to check_X_y as well ? it has pretty much the same parameters as check_array, although I don't think we ever use it in conjonction with check_non_negative.

[jeremiedbb]

LGTM

[glemaitre]

@glemaitre, should we add it to check_X_y as well ? it has pretty much the same parameters as check_array, although I don't think we ever use it in conjonction with check_non_negative.

Yep we can do that but in a subsequent PR.

[glemaitre]

OK after failing at properly using git merge, I think this is all good ;)

[glemaitre]

Enabling auto-merge. Thanks @TamaraAtanasoska