[MRG+1] idx_under should be expressed by indices of parameter X #220
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
glemaitre
requested changes
Jan 17, 2017
| # If we need to offer support for the indices | ||
| if self.return_indices: | ||
| idx_under = np.flatnonzero(y == self.min_c_) | ||
| idx_maj = np.flatnonzero(y == self.maj_c_) |
There was a problem hiding this comment.
This will make the algorithm failed in the case of multiclass.
idx_maj needs to be defined for each class in the for loop.
Subsequently, the rest of the algorithm needs to take it into account.
|
@osanai-hisashi We want to keep the OSS to tackle the multiclass problem. So the solution still need some work to be compliant, but there is a bug in the index as you mentioned. |
With current implementation idx_under is concatenated with idx_under, idx_maj_sample and idx_tmp. And idx_maj_sample is created with indices of majority class so wrong indices will be created. This patch fixes the way of creating idx_under.
osanai-hisashi
force-pushed
the
master
branch
from
39c3424 to
7e26011
Compare
|
thanks for the review. I will move L185 into the for loop with a condition change (y == key). |
glemaitre
approved these changes
Jan 17, 2017
|
LGTM @chkoar can you make the MRG+1 just to be sure |
glemaitre
changed the title
christophe-rannou
pushed a commit
to christophe-rannou/imbalanced-learn
that referenced
this pull request
Apr 3, 2017
…-contrib#220) With current implementation idx_under is concatenated with idx_under, idx_maj_sample and idx_tmp. And idx_maj_sample is created with indices of majority class so wrong indices will be created. This patch fixes the way of creating idx_under.
glemaitre
pushed a commit
to glemaitre/imbalanced-learn
that referenced
this pull request
Jun 15, 2017
…-contrib#220) With current implementation idx_under is concatenated with idx_under, idx_maj_sample and idx_tmp. And idx_maj_sample is created with indices of majority class so wrong indices will be created. This patch fixes the way of creating idx_under.
glemaitre
pushed a commit
to glemaitre/imbalanced-learn
that referenced
this pull request
Jun 15, 2017
…-contrib#220) With current implementation idx_under is concatenated with idx_under, idx_maj_sample and idx_tmp. And idx_maj_sample is created with indices of majority class so wrong indices will be created. This patch fixes the way of creating idx_under.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Reference Issue
What does this implement/fix? Explain your changes.
With current implementation idx_under is concatenated with idx_under,
idx_maj_sample and idx_tmp. And idx_maj_sample is created with
indices of majority class so wrong indices will be created.
This patch fixes the way of creating idx_under.
Any other comments?