Add return_X_y to 20_newsgroups and olivetti_faces#14259
Add return_X_y to 20_newsgroups and olivetti_faces#14259qinhanmin2014 merged 14 commits intoscikit-learn:masterfrom
Conversation
thomasjpfan
left a comment
There was a problem hiding this comment.
Please add a test in sklearn/datasets/tests/test_20news.py using check_return_X_y.
|
please either leave the issue open or add this option to fetch_olivetti_faces. |
Co-Authored-By: Thomas J Fan <thomasjpfan@gmail.com>
Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>
jnothman
left a comment
There was a problem hiding this 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
qinhanmin2014
left a comment
There was a problem hiding this comment.
you also need to update the Returns section
please add some tests
|
I pushed a couple of changes needed. We need to check the rendering of the documentation before merging. |
rth
left a comment
There was a problem hiding this comment.
Thanks! Fetchers in the following files could also be changes,
examples/model_selection/grid_search_text_feature_extraction.pyexamples/text/plot_document_clustering.pyexamples/text/plot_document_classification_20newsgroups.pyexamples/text/plot_hashing_vs_dict_vectorizer.pyexamples/bicluster/plot_bicluster_newsgroups.pyexamples/linear_model/plot_sparse_logistic_regression_20newsgroups.pyexamples/applications/plot_model_complexity_influence.pyexamples/plot_johnson_lindenstrauss_bound.pyexamples/ensemble/plot_forest_importances_faces.pyexamples/cluster/plot_dict_face_patches.py
(but as you want, I'm fine with the current version as well)
I checked them and we are using |
|
Thought I might have forgotten some. I will double-check. |
rth
left a comment
There was a problem hiding this 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.
|
I pushed the 2 missing occurrences |
|
codecov is failing because we are not downloading the dataset on the CI. However, the tests are passing locally. |
|
and the example are built |
|
I just fixed the documentation rendering. It is good to be merged. |
|
thanks @souravsingh |
Reference Issues/PRs
Fixes #14258
What does this implement/fix? Explain your changes.
Adds
return_X_yparameter to the 20_newsgroups method