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

Add return_X_y to 20_newsgroups and olivetti_faces #14259

Merged
merged 14 commits into from Jul 20, 2019

Conversation

souravsingh
Copy link
Contributor

@souravsingh souravsingh commented Jul 4, 2019

Reference Issues/PRs

Fixes #14258

What does this implement/fix? Explain your changes.

Adds return_X_y parameter to the 20_newsgroups method

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Please add a test in sklearn/datasets/tests/test_20news.py using check_return_X_y.

sklearn/datasets/twenty_newsgroups.py Outdated Show resolved Hide resolved
@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Jul 5, 2019

please either leave the issue open or add this option to fetch_olivetti_faces.

souravsingh and others added 3 commits Jul 9, 2019
Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@thomasjpfan thomasjpfan changed the title Add return_X_y to 20_newsgroups Add return_X_y to 20_newsgroups and olivetti_faces Jul 9, 2019
Copy link
Member

@jnothman jnothman left a comment

Please add simple test cases to ensure line coverage. If there are examples that use these fetch findings and only use data and target from them, please consider using this parameter in those examines. Thanks

sklearn/datasets/olivetti_faces.py Outdated Show resolved Hide resolved
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

you also need to update the Returns section
please add some tests

@glemaitre glemaitre self-assigned this Jul 19, 2019
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

I pushed a couple of changes needed. We need to check the rendering of the documentation before merging.

Copy link
Member

@rth rth left a comment

Thanks! Fetchers in the following files could also be changes,

  • examples/model_selection/grid_search_text_feature_extraction.py
  • examples/text/plot_document_clustering.py
  • examples/text/plot_document_classification_20newsgroups.py
  • examples/text/plot_hashing_vs_dict_vectorizer.py
  • examples/bicluster/plot_bicluster_newsgroups.py
  • examples/linear_model/plot_sparse_logistic_regression_20newsgroups.py
  • examples/applications/plot_model_complexity_influence.py
  • examples/plot_johnson_lindenstrauss_bound.py
  • examples/ensemble/plot_forest_importances_faces.py
  • examples/cluster/plot_dict_face_patches.py
    (but as you want, I'm fine with the current version as well)

sklearn/datasets/tests/test_20news.py Show resolved Hide resolved
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

Thanks! Fetchers in the following files could also be changes,

I checked them and we are using target_names or other keys from the bunch and therefore we shall let the example as they are.

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

Thought I might have forgotten some. I will double-check.

rth
rth approved these changes Jul 19, 2019
Copy link
Member

@rth rth left a comment

I checked them and we are using target_names or other keys from the bunch and therefore we shall let the example as they are.

Fair enough. I don't think it's very critical anyway, having some examples using Bunch is also useful.

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

I pushed the 2 missing occurrences

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

codecov is failing because we are not downloading the dataset on the CI. However, the tests are passing locally.

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

and the example are built

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Jul 19, 2019

I just fixed the documentation rendering. It is good to be merged.

rth
rth approved these changes Jul 19, 2019
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Minor nits

examples/plot_multioutput_face_completion.py Outdated Show resolved Hide resolved
examples/decomposition/plot_faces_decomposition.py Outdated Show resolved Hide resolved
examples/text/plot_hashing_vs_dict_vectorizer.py Outdated Show resolved Hide resolved
sklearn/datasets/olivetti_faces.py Outdated Show resolved Hide resolved
sklearn/datasets/tests/test_20news.py Show resolved Hide resolved
sklearn/datasets/tests/test_olivetti_faces.py Outdated Show resolved Hide resolved
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Whats new missing

@qinhanmin2014 qinhanmin2014 merged commit f8e4cc4 into scikit-learn:master Jul 20, 2019
12 of 14 checks passed
@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Jul 20, 2019

thanks @souravsingh

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.

6 participants