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

[MRG] Deterministic order for load_sample_images #13250

Merged
merged 13 commits into from Feb 26, 2019

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Feb 25, 2019

Reference Issues/PRs

Fixes #13152
Fixes #13250
Closes #13178
Closes #13249

What does this implement/fix? Explain your changes.

This PR makes sure that the output of os.listdir has a deterministic order in load_sample_images.

Any other comments?

Sometimes the "flower.jpg" image is used instead of "china.jpg" in the extract_patches_2d's doctest.

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 25, 2019

I think it's better than #13249
please add the print statement back, see #13238

Copy link
Member

@rth rth left a comment

Looks good.

please add the print statement back, see #13238

Maybe revert #13238 altogether?

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 25, 2019

Maybe revert #13238 altogether?

this is what I mean :)

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 25, 2019

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:.

@@ -523,6 +534,3 @@ def transform(self, X):
patches[ii * n_patches:(ii + 1) * n_patches] = extract_patches_2d(
image, patch_size, self.max_patches, self.random_state)
return patches

def _more_tags(self):

This comment has been minimized.

@@ -225,6 +225,9 @@ Changelog
- |Fix| :func:`datasets.fetch_openml` to retry downloading when reading
from local cache fails. :issue:`12517` by :user:`Thomas Fan <thomasjpfan>`.

- |Fix| :func:`datasets.load_sample_images` returns images with a deterministic

This comment has been minimized.

@qinhanmin2014

qinhanmin2014 Feb 25, 2019
Member

Seems that Joel labelled the issues as 0.20.3, so I think it's acceptable.

This comment has been minimized.

@jnothman

jnothman Feb 25, 2019
Member

I think I labelled it as 0.20.3 by mistake. The doctest isn't in 0.20. I'd rather this fix in 0.21

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Feb 25, 2019

@thomasjpfan please update the PR body to close all related issues/PRs

@jnothman
Copy link
Member

@jnothman jnothman commented Feb 25, 2019

Sorry didn't see this and dupicated the issue.

@jnothman
Copy link
Member

@jnothman jnothman commented Feb 25, 2019

Everything is moving fast! I think #13249 is still the right fix for the doctest, as it was ugly to just extract [0] from all images anyway. Better to load a specific image. But this should be fixed too.

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Feb 25, 2019

For the doctest, #13249 is better.

This (small) fix is still needed to make sure load_sample_images has the same order. Since the doctest is removed, a test_load_sample_images_correct_order has been added.

@glemaitre glemaitre merged commit b0ab289 into scikit-learn:master Feb 26, 2019
18 checks passed
18 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100% of diff hit (target 92.48%)
Details
@codecov
codecov/project 92.48% (+<.01%) compared to 314686a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20190225.106 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py35_np_atlas) Linux py35_np_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_conda) Linux pylatest_conda succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py35) Windows py35 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37) Windows py37 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda) macOS pylatest_conda succeeded
Details
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde added a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment