MAINT use nanmin to replace nan by finite values in ranking of SearchCV #24543
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<
8000
/div>
|
This PR should solve the remaining failure shown in the CIs using the dev versions. |
thomasjpfan
reviewed
Sep 29, 2022
|
Done. It will still fail until #24521 is in |
|
I just merged #24521. |
|
OK merged |
thomasjpfan
reviewed
Sep 29, 2022
There was a problem hiding this comment.
Overall, the behavior on with scikit-learn 1.1 and SciPy 1.8 is:
from scipy.stats import rankdata
import numpy as np
array_means = np.asarray([np.nan] * 3)
min_array_means = min(array_means) - 1
array_means = np.nan_to_num(array_means, copy=True, nan=min_array_means)
rankdata(-array_means, method="min")
# array([1, 2, 3])With this PR and the nanmin fix:
from scipy.stats import rankdata
import numpy as np
array_means = np.asarray([np.nan] * 3)
min_array_means = np.nanmin(array_means) - 1
if np.isnan(min_array_means):
min_array_means = 0
array_means = np.nan_to_num(array_means, copy=True, nan=min_array_means)
rankdata(-array_means, method="min")
# array([1, 1, 1])Even tho this is a edge case, it is backward breaking change. If we move forward with this behavior change, I think it belongs in the change log.
Furthermore, for array_means = np.asarray([np.nan] * 3 + [1, 2, 3]), the rank_results are:
- SciPy 1.8 and on scikit-learn 1.1 ->
rank_results=array([4, 4, 4, 3, 2, 1]) - SciPy 1.10 and
nanminfix ->rank_results=array([4, 5, 6, 3, 2, 1])
sklearn/model_selection/_search.py
Outdated
| @@ -968,7 +968,10 @@ def _store(key_name, array, weights=None, splits=False, rank=False): | |||
| # when input is nan, scipy >= 1.10 rankdata returns nan. To | |||
| # keep previous behaviour nans are set to be smaller than the | |||
| # minimum value in the array before ranking | |||
| min_array_means = min(array_means) - 1 | |||
| min_array_means = np.nanmin(array_means) - 1 | |||
There was a problem hiding this comment.
When array_means is all nan, there is a RuntimeWarning:
RuntimeWarning: All-NaN slice encountered
There was a problem hiding this comment.
We can either catch it or check for all nans first before calling nanmin.
There was a problem hiding this comment.
I catch this case early.
|
Ranking only nan as no meaning and we should not give a ranking IMO.
I would be fine with the current incompatibility.
We can always get this behaviour and make it a bug fix with a change log entry. WDYT?
…Sent from my iPhone
On 29 Sep 2022, at 19:16, Thomas J. Fan ***@***.***> wrote:
@thomasjpfan commented on this pull request.
Overall, the behavior on with scikit-learn 1.1 and SciPy 1.8 is:
from scipy.stats import rankdata
import numpy as np
array_means = np.asarray([np.nan] * 3)
min_array_means = min(array_means) - 1
array_means = np.nan_to_num(array_means, copy=True, nan=min_array_means)
rankdata(-array_means, method="min")
# array([1, 2, 3])
With this PR and the nanmin fix:
from scipy.stats import rankdata
import numpy as np
array_means = np.asarray([np.nan] * 3)
min_array_means = np.nanmin(array_means) - 1
if np.isnan(min_array_means):
min_array_means = 0
array_means = np.nan_to_num(array_means, copy=True, nan=min_array_means)
rankdata(-array_means, method="min")
# array([1, 1, 1])
Even tho this is a edge case, it is backward breaking change. If we move forward with this behavior change, I think it belongs in the change log.
Furthermore, for array_means = np.asarray([np.nan] * 3 + [1, 2, 3]), the rank_results are:
SciPy 1.8 and on scikit-learn 1.1 -> rank_results=array([4, 4, 4, 3, 2, 1])
SciPy 1.10 and nanmin fix -> rank_results=array([4, 5, 6, 3, 2, 1])
In sklearn/model_selection/_search.py:
> @@ -968,7 +968,10 @@ def _store(key_name, array, weights=None, splits=False, rank=False):
# when input is nan, scipy >= 1.10 rankdata returns nan. To
# keep previous behaviour nans are set to be smaller than the
# minimum value in the array before ranking
- min_array_means = min(array_means) - 1
+ min_array_means = np.nanmin(array_means) - 1
When array_means is all nan, there is a RuntimeWarning:
RuntimeWarning: All-NaN slice encountered
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
I am okay with making it a bug fix and adding it to the change log. |
betatim
reviewed
Sep 30, 2022
betatim
reviewed
Sep 30, 2022
ogrisel
reviewed
Sep 30, 2022
Co-authored-by: Tim Head <betatim@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
ogrisel
reviewed
Oct 13, 2022
|
And also please re-push a commit for |
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
|
I modified the previous test to assert the new behaviour. It would fail on |
|
@ogrisel It is green ;) |
lesteve
reviewed
Oct 13, 2022
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
|
Thanks @lesteve. I will be merging since I don't run the right CI anyway. |
glemaitre
added a commit
to glemaitre/scikit-learn
that referenced
this pull request
Oct 31, 2022
…CV (scikit-learn#24543) Co-authored-by: Tim Head <betatim@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #24424
For SciPy >= 1.10,
rankdatadoes not deal withnan.When finding the minimum finite values, we should be using
np.nanminand notminto discardnanvalues. The subsequent casting is therefore safe.