Skip to content

[MRG] Deterministic order for load_sample_images #13250

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

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

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.

Looks good.

please add the print statement back, see #13238

Maybe revert #13238 altogether?

@qinhanmin2014
Copy link
Member

Maybe revert #13238 altogether?

this is what I mean :)

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

@@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

?

@@ -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
Copy link
Member

@qinhanmin2014 qinhanmin2014 Feb 25, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

@jnothman
Copy link
Member

Sorry didn't see this and dupicated the issue.

@jnothman
Copy link
Member

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

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

failure: [doctest] sklearn.feature_extraction.image.extract_patches_2d
5 participants