-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[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
Conversation
pep8 errors |
You need to add a test case to sklearn/datasets/tests/test_openml.py |
Both done. |
sklearn/datasets/openml.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @vufg
sklearn/datasets/openml.py
Outdated
@@ -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: |
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do.
Please add an entry to the change log at |
I don't think it's useful to the user to have a what's new entry for this
given that the function is not released
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @vufg !
There was a problem hiding this 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!
OK, I'm on it. |
I'll merge this one and open a PR myself to update the examples. Thanks @vufg |
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?