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] Support for list of lists or list of arrays multilabel indicator (continuation) #14865

Merged
merged 33 commits into from Oct 25, 2019

Conversation

leonardbinet
Copy link
Contributor

@leonardbinet leonardbinet commented Sep 1, 2019

Reference Issues/PRs

Rebase of #9158 to solve #7931

What does this implement/fix? Explain your changes.

Any other comments?

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 1, 2019

Yes, let's do this!

Copy link
Member

@jnothman jnothman left a comment

Test failures...

sklearn/utils/multiclass.py Outdated Show resolved Hide resolved
@leonardbinet leonardbinet changed the title [WIP] Support for list of lists or list of arrays multilabel indicator (continuation) [MRG] Support for list of lists or list of arrays multilabel indicator (continuation) Sep 2, 2019
@leonardbinet
Copy link
Contributor Author

@leonardbinet leonardbinet commented Sep 2, 2019

@jnothman, fixed it

@jnothman jnothman added High Priority Waiting for Reviewer labels Sep 5, 2019
Copy link
Member

@jnothman jnothman left a comment

Thanks, it's looking pretty good.

Please add an Enhancement entry to the change log at doc/whats_new/v0.22.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

sklearn/preprocessing/tests/test_label.py Outdated Show resolved Hide resolved
sklearn/metrics/tests/test_common.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_estimator_checks.py Outdated Show resolved Hide resolved
@jnothman
Copy link
Member

@jnothman jnothman commented Sep 8, 2019

Please append commits rather than force pushing. Much easier to see changes between reviews that way

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
:pr:`14336` by :user:`Gregory Dexter <gdex1>`.

- |Enhancement| label_binarize now supports list of lists for multilabel data
Copy link
Member

@jnothman jnothman Sep 8, 2019

Choose a reason for hiding this comment

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

Might be more helpful to advertise the changes in terms of how they affect metrics and other users of type_of_target

Copy link
Contributor Author

@leonardbinet leonardbinet Sep 8, 2019

Choose a reason for hiding this comment

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

how would you put it? one entry per impacted function or a more generic explanation on what is considered as multiclass-multioutput/multilabel-indicator?

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 8, 2019

I've not looked at the code or codecov's complaint this time

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 8, 2019

sklearn/preprocessing/tests/test_label.py Outdated Show resolved Hide resolved
sklearn/metrics/tests/test_common.py Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
@leonardbinet
Copy link
Contributor Author

@leonardbinet leonardbinet commented Sep 9, 2019

thanks for your help and patience @jnothman, you're the victim of my first contribution to scikit-learn 😅

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 9, 2019

I'm very happy you're finishing this off. It's something we should have done years ago, having removed sequence of sequence support around 2015

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 10, 2019

sklearn/preprocessing/tests/test_label.py:27:1: F401 'sklearn.utils.testing.assert_raises' imported but unused
from sklearn.utils.testing import assert_raises, assert_raise_message

Copy link
Member

@jnothman jnothman left a comment

Otherwise lgtm

sklearn/preprocessing/tests/test_label.py Outdated Show resolved Hide resolved
@jnothman
Copy link
Member

@jnothman jnothman commented Sep 10, 2019

It looks like you've introduced a syntax error. And it's worth looking at the codecov error

@jnothman
Copy link
Member

@jnothman jnothman commented Sep 10, 2019

Maybe merging in the latest master will fix the codecov issue

Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks @leonardbinet , only super nitpicks from me

The PR has merge conflicts but apart from that LGTM

sklearn/utils/multiclass.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@@ -227,7 +228,7 @@ def type_of_target(y):
>>> type_of_target(np.array([[1, 2], [3, 1]]))
'multiclass-multioutput'
>>> type_of_target([[1, 2]])
'multiclass-multioutput'
'multilabel-indicator'
Copy link
Member

@NicolasHug NicolasHug Oct 16, 2019

Choose a reason for hiding this comment

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

question: is this a bugfix?

Copy link
Member

@jnothman jnothman Oct 17, 2019

Choose a reason for hiding this comment

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

It is more-or-less a bug fix. Either way, type_of_target is expected to return "the most specific type that can be inferred". Multilabel is a specific subtype of multiclass-multioutput. So this is correct

@jnothman
Copy link
Member

@jnothman jnothman commented Oct 23, 2019

@leonardbinet please resolve conflicts with master and address @NicolasHug's teeny weeny comments so that we can merge this for release. If that's looking unlikely in the next week or so, please let us know.

@leonardbinet
Copy link
Contributor Author

@leonardbinet leonardbinet commented Oct 24, 2019

@jnothman @NicolasHug sure, thanks for your feedbacks I'll fix this this week-end

@leonardbinet leonardbinet force-pushed the fix_is_multilabel branch 7 times, most recently from f0d8b94 to a71cac1 Compare Oct 24, 2019
@jnothman
Copy link
Member

@jnothman jnothman commented Oct 25, 2019

@leonardbinet, force pushing makes it much harder for reviewers to track what changes have occurred since last review.

@jnothman jnothman merged commit 846e6a3 into scikit-learn:master Oct 25, 2019
18 of 19 checks passed
@jnothman
Copy link
Member

@jnothman jnothman commented Oct 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority Waiting for Reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants