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+2] adding multilabel support for score_func #7676

Merged
merged 4 commits into from Oct 20, 2016

Conversation

Projects
None yet
4 participants
@affanv14
Contributor

affanv14 commented Oct 15, 2016

Reference Issue

FIxes #7628

What does this implement/fix? Explain your changes.

As said in comments that y can take any form it likes in score_func,I changed multi_output in check_X_y to true to fix this.

Any other comments?

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 15, 2016

I'm tempted to say we should leave all validation of X and y to score_func. Tests needed. And perhaps documentation changes.

@affanv14

This comment has been minimized.

Contributor

affanv14 commented Oct 16, 2016

So i need to write a test to show chi2 works on multilabel right?

@amueller

This comment has been minimized.

Member

amueller commented Oct 17, 2016

@affanv14 yes

@affanv14

This comment has been minimized.

Contributor

affanv14 commented Oct 19, 2016

I added a test which i copied from another test and made some changes to.Does this suffice?

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 19, 2016

I'm okay with this, but I'd rather support arbitrary score_func, drop check_X_y, and come up with some test case that ensures that a y that wouldn't be acceptable for any of the in-built functions can be accepted by a custom score_func...

@cammil

This comment has been minimized.

cammil commented Oct 19, 2016

Is it worth me collaborating still? If so how? Not sure how it would work with two people working on the same code.

I don't yet fully understand the code so will keep reading until further notice!

@jnothman jnothman added the Bug label Oct 19, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 19, 2016

I suspect this issue is too small to have two people working on it. I also think it could be merged as it is (if we want it in 0.18.1, @amueller ), but I would like the constraints to be looser eventually.

@cammil

This comment has been minimized.

cammil commented Oct 19, 2016

@affanv14 I think you're further along with the fix than I am, so I suggest you continue. If you're not going to implement the full fix (looser constraints), I could work on that. Let me know.

@affanv14

This comment has been minimized.

Contributor

affanv14 commented Oct 19, 2016

@cammil I actually have my tests coming up.So i would'nt be able to work on it for a week or so.You can work on it instead.

@cammil

This comment has been minimized.

cammil commented Oct 19, 2016

@affanv14 Will see what I can do ...

@amueller amueller added this to the 0.18.1 milestone Oct 19, 2016

@amueller

This comment has been minimized.

Member

amueller commented Oct 19, 2016

@jnothman what requirements would you like to see?
We could actually remove check_X_y and it would mostly work.
SelectKBest uses X.shape[1] which might break on list-input. In principle we could just check there?
However, if a user provided a custom function and didn't do conversion to arrays in their function, their code will break if we make the validation less strict. That happened quite a bunch to people when train_test_split stopped casting lists to arrays.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 20, 2016

Fair enough. Let's merge it as is, then.

On 20 October 2016 at 06:07, Andreas Mueller notifications@github.com
wrote:

@jnothman https://github.com/jnothman what requirements would you like
to see?
We could actually remove check_X_y and it would mostly work.
SelectKBest uses X.shape[1] which might break on list-input.
However, if a user provided a custom function and didn't do conversion to
arrays in their function, their code will break if we make the validation
less strict. That happened quite a bunch to people when train_test_split
stopped casting lists to arrays.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7676 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz696OWmfj9GzZdFkjZWxMi4L5zZ5Aks5q1mp3gaJpZM4KXpI0
.

@jnothman jnothman changed the title from [WIP]adding multilabel support for score_func to [MRG+1] adding multilabel support for score_func Oct 20, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 20, 2016

(With your assent, of course.)

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 20, 2016

And a what's new.

@amueller

This comment has been minimized.

Member

amueller commented Oct 20, 2016

@affanv14 can you please add an entry to doc/whatsnew.rst explaining your changes?

@amueller amueller changed the title from [MRG+1] adding multilabel support for score_func to [MRG+2] adding multilabel support for score_func Oct 20, 2016

@amueller

This comment has been minimized.

Member

amueller commented Oct 20, 2016

lgtm

@affanv14

This comment has been minimized.

Contributor

affanv14 commented Oct 20, 2016

Finished it.

@@ -69,7 +69,11 @@ Bug fixes
`#6497 <https://github.com/scikit-learn/scikit-learn/pull/6497>`_
by `Sebastian Säger`_
- Fixes issue in :class:`_BaseFilter` where score functions were

This comment has been minimized.

@amueller

amueller Oct 20, 2016

Member

The link will not work because it doesn't include the module, also the class is private, so there's no documentation to link to. Maybe do a :ref:univariate_feature_selection`` instead?

@amueller amueller merged commit ee3e617 into scikit-learn:master Oct 20, 2016

1 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@amueller

This comment has been minimized.

Member

amueller commented Oct 20, 2016

thanks!

amueller added a commit to amueller/scikit-learn that referenced this pull request Oct 25, 2016

[MRG+2] adding multilabel support for score_func (scikit-learn#7676)
* added multilabel support for score function

* added test for multilabel score function

* updated whats_new.rst

* updated whats_new.rst with working link

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

[MRG+2] adding multilabel support for score_func (scikit-learn#7676)
* added multilabel support for score function

* added test for multilabel score function

* updated whats_new.rst

* updated whats_new.rst with working link

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+2] adding multilabel support for score_func (scikit-learn#7676)
* added multilabel support for score function

* added test for multilabel score function

* updated whats_new.rst

* updated whats_new.rst with working link

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+2] adding multilabel support for score_func (scikit-learn#7676)
* added multilabel support for score function

* added test for multilabel score function

* updated whats_new.rst

* updated whats_new.rst with working link

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+2] adding multilabel support for score_func (scikit-learn#7676)
* added multilabel support for score function

* added test for multilabel score function

* updated whats_new.rst

* updated whats_new.rst with working link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment