-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[MRG + 1] Fix ValueError in LabelEncoder when using inverse_transform on unseen labels #9816
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
@@ -148,8 +149,9 @@ def inverse_transform(self, y): | |||
check_is_fitted(self, 'classes_') | |||
|
|||
diff = np.setdiff1d(y, np.arange(len(self.classes_))) | |||
if diff: | |||
raise ValueError("y contains new labels: %s" % str(diff)) | |||
if len(diff): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This len is the fix, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - that's all that was needed to produce a sensible error 😄
assert_raises(ValueError, le.inverse_transform, [-1]) | ||
le.fit([1, 2, 3, -1, 1]) | ||
msg = "contains previously unseen labels" | ||
assert_raise_message(ValueError, msg, le.inverse_transform, [-2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the organization of the tests a bit weird but not your fault. The test that it actually works if they are present is way at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to reorganise tomorrow if you are able to give me some pointers - I'm not very familiar with the testing structure of sklearn as this is my first issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine, I think.
LGTM, merging, thanks a lot @newey01c! |
This is missing a whats_new entry. I'll pull it into my 0.19.1 branch and write an entry there |
The issue appears to be persistent - I am using LabelEncoder. Here is my stack trace:
Do you have any suggestions? |
As noted in #10552, this was accidentally not included in the 0.19.1 release. Using the development version of scikit-learn will make it work. |
I tried to follow the instructions on the website, but I could not import
any scikit-learn libraries. I also ran pip install -e, but I got the
following error: IndexError: list index out of range. Do you have any
pointers? I am using Python 2.7 on Ubuntu 16.04.
On Sat, Feb 3, 2018 at 6:41 PM Joel Nothman ***@***.***> wrote:
As noted in #10552
<#10552>, this was
accidentally not included in the 0.19.1 release. Using the development
version of scikit-learn will make it work.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9816 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AV4hUWvS5UzS3P1532AQtZOjXe8bnh6Sks5tRO68gaJpZM4PfUrv>
.
--
Vijay
|
not sure without a full traceback what index was out of range. you can try
pip install https://g <https://hith>
ithub.com/scikit-learn/scikit-learn/archive/master.zip to install the
latest development version.
… |
Thank you - I found an alternate solution.
On Sun, Feb 4, 2018 at 5:00 PM Joel Nothman ***@***.***> wrote:
not sure without a full traceback what index was out of range. you can try
pip install https://g <https://hith>
ithub.com/scikit-learn/scikit-learn/archive/master.zip to install the
latest development version.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9816 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AV4hUXCZMFcf1ARIl9XGIRyNj2IdsGZDks5tRihmgaJpZM4PfUrv>
.
--
Vijay
|
Reference Issue
Fix #9812