Skip to content

[MRG+1] fetch_openml should support return_X_y #11840

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

Merged
merged 11 commits into from
Aug 19, 2018
Merged

[MRG+1] fetch_openml should support return_X_y #11840

merged 11 commits into from
Aug 19, 2018

Conversation

panzhufeng
Copy link
Contributor

@panzhufeng panzhufeng commented Aug 16, 2018

Reference Issues/PRs

Fixes #11814. See also #11466.

What does this implement/fix? Explain your changes.

Modify fetch_openml to support return_X_y parameter.
Add return_X_y test in test_openml.py

Any other comments?

@panzhufeng panzhufeng changed the title [MRG] fetch_openml should support return_X_y [WIG] fetch_openml should support return_X_y Aug 16, 2018
@panzhufeng panzhufeng changed the title [WIG] fetch_openml should support return_X_y [MRG] fetch_openml should support return_X_y Aug 16, 2018
@panzhufeng panzhufeng changed the title [MRG] fetch_openml should support return_X_y [WIG] fetch_openml should support return_X_y Aug 16, 2018
@panzhufeng panzhufeng changed the title [WIG] fetch_openml should support return_X_y [MRG] fetch_openml should support return_X_y Aug 16, 2018
@amueller
Copy link
Member

pep8 errors

@amueller amueller added this to the 0.20 milestone Aug 16, 2018
@jnothman
Copy link
Member

You need to add a test case to sklearn/datasets/tests/test_openml.py

@panzhufeng
Copy link
Contributor Author

Both done.

@@ -395,6 +395,10 @@ def fetch_openml(name=None, version='active', data_id=None, data_home=None,
cache : boolean, default=True
Whether to cache downloaded datasets using joblib.

return_X_y : boolean, default=False.
If True, returns ``(data, target)`` instead of a Bunch object.
See below for more information about the `data` and `target` object
Copy link
Member

Choose a reason for hiding this comment

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

No extra indentation is needed here (the "See .." should align with "If True ...")

object -> objects

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @vufg

@@ -563,4 +569,7 @@ def fetch_openml(name=None, version='active', data_id=None, data_home=None,
categories=nominal_attributes,
url="https://www.openml.org/d/{}".format(data_id))

if return_X_y:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put it above bunch = Bunch(...)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please do.

@qinhanmin2014 qinhanmin2014 changed the title [MRG] fetch_openml should support return_X_y [MRG+1] fetch_openml should support return_X_y Aug 18, 2018
@qinhanmin2014
Copy link
Member

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

@jnothman
Copy link
Member

jnothman commented Aug 18, 2018 via email

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @vufg !

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

#11466 has been merged, would you mind merging master in to get those changes and adapting the relevant examples from that PR to use return_X_y as suggested in the parent issue description? Thanks!

@panzhufeng
Copy link
Contributor Author

OK, I'm on it.

@qinhanmin2014
Copy link
Member

I'll merge this one and open a PR myself to update the examples. Thanks @vufg

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.

fetch_openml should support return_X_y
6 participants