Make sure X and y are sorted equally in select_features #715
Conversation
This file contains 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
…regation test against it
|
You have style errors. See them below. ./tests/integrations/examples/test_driftbif_simulation.py:73:121: E501 line too long (127 > 120 characters) |
| @@ -140,6 +140,11 @@ def select_features(X, y, test_for_binary_target_binary_feature=defaults.TEST_FO | |||
| if isinstance(y, np.ndarray): | |||
| y = pd.Series(y, index=X.index) | |||
|
|
|||
| y = y.sort_index() | |||
There was a problem hiding this comment.
I would move the sorting inside the calculate_relevance_table method. That way it gets more robust
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.
This looks like a quite large bug to me.
In principle, X and y could have the same index entries (what we check), but we never check if the order is the same! So passing in a
Xwith index[0, 1, 2]and aywith index[2, 1, 0]will not raise any problems.For classification tasks, this is not a problem, as all the significance tests except
target_real_feature_real_testdo not care about the order of the index, asxandyare re-ordered internally again in all the other tasks. But fortarget_real_feature_real_testthe index is stripped before any re-order could happen - which leads to the bug described in #713.This PR fixes this. I am a bit puzzled why this was never a problem before :-/
Therefore I added the exact test from the issue, to see any degradation again.
@MaxBenChrist as a general remark: we should add more tests on
select_features:-)