Skip to content
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

[MRG+1]Corrected OvO partial_fit to work in case labels are missing in some call #6250

Closed
wants to merge 3 commits into from

Conversation

@kaichogami
Copy link
Contributor

@kaichogami kaichogami commented Jan 29, 2016

Added tests of various scenarios, raised error in case mini-batch doesn't contain classes from all_classes.

Added tests of various scenarios, raised error in case mini-batch doesn't contain classes from all_classes.
@kaichogami
Copy link
Contributor Author

@kaichogami kaichogami commented Jan 31, 2016

@MechCoder @GaelVaroquaux please review.

@kaichogami kaichogami changed the title Corrected OvO partial_fit to work in various test cases [MRG]Corrected OvO partial_fit to work in various test cases Feb 20, 2016
@kaichogami
Copy link
Contributor Author

@kaichogami kaichogami commented Feb 22, 2016

@MechCoder please have a look at this when you are free. This was a follow up PR from #6200 for the OvO partial_fit method in case all classes are not present in some call.

@@ -513,6 +514,11 @@ def partial_fit(self, X, y, classes=None):
range(self.n_classes_ *
(self.n_classes_-1) // 2)]

if not set(self.classes_).issuperset(y):

This comment has been minimized.

@MechCoder

MechCoder Feb 22, 2016
Member

If you are using np.unique(y) below, you might as well store np.unique(y) and raise an Error if not all(np.in1d(np.unique(y), self.classes_)

@@ -513,6 +514,11 @@ def partial_fit(self, X, y, classes=None):
range(self.n_classes_ *
(self.n_classes_-1) // 2)]

if not set(self.classes_).issuperset(y):
raise ValueError("Mini-batch contains {0} while classes "

This comment has been minimized.

@MechCoder

MechCoder Feb 22, 2016
Member

I would just write while it must be a subset of, I don't think the word classes is appropriate here.


#raises error when mini-batch does not have classes from all_classes
ovo = OneVsOneClassifier(MultinomialNB())
assert_raises(ValueError, ovo.partial_fit, X[:7], [0, 1, 2, 3, 4, 5, 2],

This comment has been minimized.

@MechCoder

MechCoder Feb 22, 2016
Member

We should use assert_raises_regexp here, It raises a ValueError in master as well.

@MechCoder
Copy link
Member

@MechCoder MechCoder commented Feb 22, 2016

LGTM pending minor comments

@MechCoder
Copy link
Member

@MechCoder MechCoder commented Feb 22, 2016

Can you update the PR title to something more reasonable? For instance, "Corrected OvO partial_fit to work when each minibatch does not contain all labels"

#raises error when mini-batch does not have classes from all_classes
ovo = OneVsOneClassifier(MultinomialNB())
error_y = [0, 1, 2, 3, 4, 5, 2]
assert_raises_regexp(ValueError, "Mini-batch contains {0} while it must be "

This comment has been minimized.

@kaichogami

kaichogami Feb 23, 2016
Author Contributor

@MechCoder This is failing when I use nosetests even though error message is exactly the same. I am guessing it has to do with some sort of formatting like used in regexp here. I was not able to find any doc listing the formatting rules.

@kaichogami kaichogami changed the title [MRG]Corrected OvO partial_fit to work in various test cases [MRG]Corrected OvO partial_fit to work in case labels are missing in some call Feb 23, 2016
@MechCoder
Copy link
Member

@MechCoder MechCoder commented Feb 23, 2016

Use re.escape around the string you want to test.

Thanks to @vighneshbirodkar for debugging that!

@kaichogami kaichogami force-pushed the kaichogami:OvO_partial branch from cedb1d9 to 75d8d67 Feb 23, 2016
@kaichogami
Copy link
Contributor Author

@kaichogami kaichogami commented Feb 23, 2016

Thanks @MechCoder and @vighneshbirodkar. This looks good now. :)

@MechCoder MechCoder changed the title [MRG]Corrected OvO partial_fit to work in case labels are missing in some call [MRG+1]Corrected OvO partial_fit to work in case labels are missing in some call Feb 23, 2016
@amueller
Copy link
Member

@amueller amueller commented Oct 8, 2016

Could you please rebase? Sorry for the slow reply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants