[MRG + 1] FIX raise an error message when n_groups > number of groups (#7681) by polmauri · Pull Request #7683 · scikit-learn/scikit-learn

polmauri · 2016-10-17T03:42:18Z

This change fixes issue #7681:

Raise ValueError when n_groups > actual number of unique groups in LeaveOneGroupOut and LeavePGroupsOut.
Add unit test.

nelson-liu · 2016-10-17T04:35:11Z

sklearn/model_selection/_split.py

        unique_groups = np.unique(groups)
+        if self.n_groups > len(unique_groups):
+            raise ValueError("Cannot have n_groups=%d greater than the number "
+                             "of unique groups: %d."


i find it odd that you choose to use = to indicate the variable value in for n_groups and : for the number of unique groups; maybe keep it consistent?

I copied the format from this other error in the same file, should I change it?

If all the file use =, you can keep =.

I think it's okay as n_groups is a parameter and unique groups is the information we are trying to include in the error message...

raghavrv · 2016-10-17T14:46:01Z

Thanks for the PR @polmauri

raghavrv · 2016-10-17T14:47:33Z

sklearn/model_selection/_split.py

        unique_groups = np.unique(groups)
+        if self.n_groups > len(unique_groups):
+            raise ValueError("Cannot have n_groups=%d greater than the number "
+                             "of unique groups: %d."


I think it's okay as n_groups is a parameter and unique groups is the information we are trying to include in the error message...

raghavrv · 2016-10-17T14:49:14Z

sklearn/model_selection/tests/test_split.py

+        list(LeavePGroupsOut(n_groups=5).split(X, y, groups))
+
+    assert_raises(ValueError, leave_one_group_out_zero_groups)
+    assert_raises(ValueError, leave_p_groups_out_greater_than_unique_groups)


Instead of defining functions you could pass the parameters to assert_raise_message / assert_raises and test these like

assert_raise_message(ValueError, "<snippet of the error message">, LeaveOneGroupOut().split, X, y)

raghavrv · 2016-10-17T14:50:06Z

sklearn/model_selection/tests/test_split.py

    assert_equal(3, LeaveOneGroupOut().get_n_splits(X, y, groups))


+def test_leave_group_out_split_errors():


--> test_leave_one_p_group_out_error_on_fewer_number_of_groups

tguillemot · 2016-10-17T14:52:30Z

Thanks @polmauri.
+1 once corrected.

tguillemot · 2016-10-17T08:53:57Z

sklearn/model_selection/tests/test_split.py

+        X = y = groups = np.ones(10)
+        list(LeavePGroupsOut(n_groups=5).split(X, y, groups))
+
+    assert_raises(ValueError, leave_one_group_out_zero_groups)


Could you add the ValueError message raised by leave_one_group_out_zero_groups to be sure it is the right message.

tguillemot · 2016-10-17T08:54:14Z

sklearn/model_selection/tests/test_split.py

+        list(LeavePGroupsOut(n_groups=5).split(X, y, groups))
+
+    assert_raises(ValueError, leave_one_group_out_zero_groups)
+    assert_raises(ValueError, leave_p_groups_out_greater_than_unique_groups)


Could you add the ValueError message raised by leave_one_group_out_zero_groups to be sure it is the right message.

polmauri · 2016-10-18T05:47:12Z

Changed:

Check error message with assert_raise_message
Pass parameters to assert_raise_message instead of defining functions

…cikit-learn#7681) This change addresses issue scikit-learn#7681: - Raise ValueError when n_groups > actual number of unique groups in LeaveOneGroupOut and LeavePGroupsOut. - Add unit test.

- Check error message with `assert_raise_message` - Pass parameters to `assert_raise_message` instead of defining functions

raghavrv · 2016-10-19T17:32:17Z

sklearn/model_selection/_split.py

        # We make a copy of groups to avoid side-effects during iteration
        groups = np.array(groups, copy=True)
        unique_groups = np.unique(groups)
+        if len(unique_groups) == 0:


len(unique_groups) <= 1

needs a test.

raghavrv · 2016-10-19T17:34:56Z

sklearn/model_selection/_split.py

        groups = np.array(groups, copy=True)
        unique_groups = np.unique(groups)
+        if len(unique_groups) == 0:
+            raise ValueError("Cannot have zero groups")


"The groups parameter contains fewer than 2 unique groups (%s). LeaveOneGroupOut expects at least 2." % (unique_groups)

raghavrv · 2016-10-19T17:36:11Z

sklearn/model_selection/_split.py

            raise ValueError("The groups parameter should not be None")
        groups = np.array(groups, copy=True)
        unique_groups = np.unique(groups)
+        if self.n_groups > len(unique_groups):


raghavrv · 2016-10-19T17:41:28Z

sklearn/model_selection/_split.py

        groups = np.array(groups, copy=True)
        unique_groups = np.unique(groups)
+        if self.n_groups > len(unique_groups):
+            raise ValueError("Cannot have n_groups=%d greater than the number "


"The groups parameter contains fewer than (or equal to) n_groups (%d) numbers of unique groups" " (%s). LeavePGroupsOut expects that atleast n_groups + 1 (%d) unique groups be present" % (n_groups, unique_groups, n_groups + 1)

amueller · 2016-10-24T23:33:48Z

@polmauri are you still working on this?

polmauri · 2016-10-25T08:30:18Z

Changes requested done.

raghavrv · 2016-10-25T10:14:47Z

Thanks for addressing all the comments. Looks good to me.

amueller · 2016-10-25T15:42:43Z

thanks :)

…scikit-learn#7681) (scikit-learn#7683) * FIX raise an error message when n_groups > actual number of groups (scikit-learn#7681) This change addresses issue scikit-learn#7681: - Raise ValueError when n_groups > actual number of unique groups in LeaveOneGroupOut and LeavePGroupsOut. - Add unit test. * Make requested changes - Check error message with `assert_raise_message` - Pass parameters to `assert_raise_message` instead of defining functions * Update condition and exception message

nelson-liu reviewed Oct 17, 2016

View reviewed changes

polmauri force-pushed the raise-error branch from 99e9c38 to 5a37b37 Compare October 17, 2016 04:53

raghavrv suggested changes Oct 17, 2016

View reviewed changes

tguillemot suggested changes Oct 17, 2016

View reviewed changes

raghavrv mentioned this pull request Oct 17, 2016

We should raise an error message when n_groups > actual number of groups #7681

Closed

polmauri force-pushed the raise-error branch from 5a37b37 to 8611f7b Compare October 18, 2016 04:57

FIX raise an error message when n_groups > actual number of groups (s…

54950ad

…cikit-learn#7681) This change addresses issue scikit-learn#7681: - Raise ValueError when n_groups > actual number of unique groups in LeaveOneGroupOut and LeavePGroupsOut. - Add unit test.

polmauri force-pushed the raise-error branch from 8611f7b to 54950ad Compare October 18, 2016 05:58

Make requested changes

1e6bc69

- Check error message with `assert_raise_message` - Pass parameters to `assert_raise_message` instead of defining functions

raghavrv suggested changes Oct 19, 2016

View reviewed changes

Update condition and exception message

7f818bf

raghavrv approved these changes Oct 25, 2016

View reviewed changes

raghavrv changed the title ~~FIX raise an error message when n_groups > number of groups (#7681)~~ [MRG + 1] FIX raise an error message when n_groups > number of groups (#7681) Oct 25, 2016

raghavrv added Bug Waiting for Reviewer labels Oct 25, 2016

raghavrv added this to the 0.18.1 milestone Oct 25, 2016

amueller merged commit 73d3f03 into scikit-learn:master Oct 25, 2016

		assert_equal(3, LeaveOneGroupOut().get_n_splits(X, y, groups))


		def test_leave_group_out_split_errors():

Uh oh!

Conversation

polmauri commented Oct 17, 2016 • edited by raghavrv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tguillemot commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polmauri commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raghavrv Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 24, 2016

Uh oh!

polmauri commented Oct 25, 2016

Uh oh!

raghavrv commented Oct 25, 2016

Uh oh!

amueller commented Oct 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polmauri commented Oct 17, 2016 •

edited by raghavrv

Loading

raghavrv Oct 17, 2016 •

edited

Loading

raghavrv Oct 19, 2016 •

edited

Loading