Skip to content

Solving: Incorrectly Specified Error Message (#7600)#7609

Closed
PNR-1 wants to merge 3 commits intoscikit-learn:masterfrom
PNR-1:errormessage
Closed

Solving: Incorrectly Specified Error Message (#7600)#7609
PNR-1 wants to merge 3 commits intoscikit-learn:masterfrom
PNR-1:errormessage

Conversation

@PNR-1
Copy link
Copy Markdown

@PNR-1 PNR-1 commented Oct 8, 2016

Fixes #7600

@PNR-1 PNR-1 changed the title Solving: Incorrectly Specified Error Message (Issue #7600) Solving: Incorrectly Specified Error Message (#7600) Oct 8, 2016
names.append(l)
except IndexError:
raise ValueError('features[i] must be in [0, n_features) '
raise ValueError('feature_names[i] must be in [0, n_features) '
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong fix. This was not the issue.

except IndexError:
raise ValueError('features[i] must be in [0, n_features) '
raise ValueError('feature_names[i] must be in [0, n_features) '
'but was %d' % i)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the issue: i should be features[i]

@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 8, 2016

Looks ok.

@PNR-1
Copy link
Copy Markdown
Author

PNR-1 commented Oct 9, 2016

I hope this solves the erroneous error message. (See what I did there)

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Oct 9, 2016

No, I meant: there was no issue with the error message, only with what was substituted into the %d; it should not be i

@PNR-1
Copy link
Copy Markdown
Author

PNR-1 commented Oct 9, 2016

Ohh. Sorry, I mistook it. Thanks a lot for helping me out.

@tetraptych
Copy link
Copy Markdown

I'm not sure this is resolved. The value error is being thrown by an attempt to access feature_names[i], not features[i]. When I encountered the original error, I spent ~30 minutes trying to figure out what was wrong with my features list when it was actually the length of feature_names that was the problem. The updated error message still only references features[i].

Maybe something like the following would work?

raise ValueError('features[i] must be in [0, len(feature_names) ) but was %d' % features[i])

Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also write a test that the error message is correct in the case of invalid features data?

@@ -307,7 +307,7 @@ def convert_feature(fx):
names.append(l)
except IndexError:
raise ValueError('features[i] must be in [0, n_features) '
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, perhaps @tetraptych is right that this would be clearer as len(feature_names)

@PNR-1 PNR-1 closed this Oct 13, 2016
@amueller
Copy link
Copy Markdown
Member

@Doppler010 why did you close? Do you not want to work on this any more?

@kgilliam125
Copy link
Copy Markdown
Contributor

@amueller I'll pick this up if @Doppler010 is out.

I read over the docs for this, and it looks like the intended usage is for features to contain indices between 0 and len(feature_names). I'm proposing to change the error message to
raise ValueError('features[i] must be in [0, len(feature_names) ) but was %d' % features[i]) as above unless there are further suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants