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

Show our data in the gallery #3388

Merged
merged 8 commits into from Oct 9, 2018
Merged

Conversation

sciunto
Copy link
Member

@sciunto sciunto commented Sep 3, 2018

Description

To pursue on the idea in #3384 I thought that it would be useful to show the data we are shipping with scikit-image. We could separate images for general purposes, scientific images and other specific images (like stereo).

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

For reviewers

(Don't remove the checklist below.)

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@sciunto sciunto added 📄 type: Documentation Updates, fixes and additions to documentation 💪 Work in progress labels Sep 3, 2018
@pep8speaks
Copy link

pep8speaks commented Sep 3, 2018

Hello @sciunto! Thanks for updating the PR.

Line 43:27: E226 missing whitespace around arithmetic operator

Comment last updated on October 02, 2018 at 10:02 Hours UTC

@codecov-io
Copy link

codecov-io commented Sep 3, 2018

Codecov Report

Merging #3388 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3388      +/-   ##
==========================================
+ Coverage   86.81%   86.81%   +<.01%     
==========================================
  Files         339      341       +2     
  Lines       27385    27412      +27     
==========================================
+ Hits        23773    23798      +25     
- Misses       3612     3614       +2
Impacted Files Coverage Δ
skimage/__init__.py 67.85% <0%> (-3.12%) ⬇️
skimage/feature/setup.py 17.24% <0%> (-1.28%) ⬇️
skimage/draw/_random_shapes.py 96.84% <0%> (-1.06%) ⬇️
skimage/filters/tests/test_gaussian.py 100% <0%> (ø) ⬆️
skimage/restoration/tests/test_denoise.py 99.47% <0%> (ø) ⬆️
skimage/future/__init__.py 100% <0%> (ø) ⬆️
skimage/feature/__init__.py 100% <0%> (ø) ⬆️
skimage/util/tests/test_dtype.py 100% <0%> (ø) ⬆️
skimage/util/dtype.py 90.74% <0%> (ø) ⬆️
skimage/feature/tests/test_cascade.py 100% <0%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f03a2c...7d48ee0. Read the comment docs.

@soupault soupault added this to the 0.15 milestone Sep 4, 2018
Copy link
Member

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

I like the idea of displaying pictures of available images in the gallery. My only concern is that the thumbnails will be unreadable with a large number of subplots, maybe organize the images with several rows for the general-purpose images? I had a quick look at the gallery, 6 subplots is still OK, 9 is very crowded.

#'lfw_subset',

fig, axes = plt.subplots(len(images), 1, figsize=(8, 4 * len(images)),
sharex=False, sharey=False)
Copy link
Member

Choose a reason for hiding this comment

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

isn't sharex/y False by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I can get ride of it.

for i, image in enumerate(images):
caller = getattr(data, image)
ax[i].imshow(caller())
ax[i].set_title(image)
Copy link
Member

Choose a reason for hiding this comment

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

``ax[i].axis('off')```to gain some space in a crowded figure?

Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred to keep it as it gives an idea of the image size.

@@ -0,0 +1,42 @@
"""
======================
General purpose images
Copy link
Member

Choose a reason for hiding this comment

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

General-purpose

@sciunto
Copy link
Member Author

sciunto commented Sep 6, 2018

You are absolutely right for the thumbnail. It's an issue. The idea I got in the mean time to solve this is to take one image as an example for the thumbnail, and then display them all. I'm afraid that, even if we succeed in generating a correct rendering, the future additions will destroy it anyway.

@emmanuelle
Copy link
Member

@sciunto you can do that by displaying a first image in the example, and then the whole subplots. I don't remember if sphinx-gallery takes the first or the last figure for the thumbnail, and whether it can be configured. The doc of sphinx-gallery is quite thorough, but its organization is sometimes a bit weird.

@sciunto
Copy link
Member Author

sciunto commented Sep 12, 2018

It's now ready for reviews.

@sciunto
Copy link
Member Author

sciunto commented Sep 17, 2018

@scikit-image/core Anyone for reviewing this please?

@soupault
Copy link
Member

The added gallery examples don't seem to show up under "Examples using ..." sections of skimage.data (i.e. http://scikit-image.org/docs/dev/api/skimage.data.html#examples-using-skimage-data-astronaut). Any ideas on this?

Also, I'd make the font in the image titles a bit larger.

Otherwise, LGTM!

@soupault soupault self-assigned this Sep 30, 2018
@sciunto
Copy link
Member Author

sciunto commented Oct 2, 2018

@soupault I increased the font size. Regarding the fact it doesn't show up in section, I guess it's because we use getattr, and not a direct call. A parser cannot get the call here... but i don't think it's significant miss.


from skimage import data

matplotlib.rcParams['font.size'] = 18
Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed that you don't set titles in this example. The font config doesn't really make sense then.

Copy link
Member

Choose a reason for hiding this comment

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

Other examples look great!

Copy link
Member Author

@sciunto sciunto Oct 3, 2018

Choose a reason for hiding this comment

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

It's just to keep the tics homogeneous...

@soupault
Copy link
Member

soupault commented Oct 2, 2018

I guess it's because we use getattr, and not a direct call

@sciunto Ah, you are right. That's, most likely, the case.

@soupault
Copy link
Member

soupault commented Oct 3, 2018 via email

@sciunto
Copy link
Member Author

sciunto commented Oct 3, 2018

Because we have a header for each image/set of images that already introduces the content... I can add it, but I did not feel it's necessary.

@sciunto
Copy link
Member Author

sciunto commented Oct 9, 2018

@emmanuelle I'm confused. Where do I call data.__all__? I couldn't reproduce.

@emmanuelle
Copy link
Member

@sciunto sorry I got mixed up (with one of my local files). Sorry about that, ignore my comment!

@emmanuelle
Copy link
Member

LGTM. It could be good to order the sections of the gallery so that the section on data appears on top (as described here https://sphinx-gallery.readthedocs.io/en/stable/advanced_configuration.html?highlight=sorting#sub-gallery-order). But we can also merge this PR first and do the ordering later, depending on your time.

@sciunto
Copy link
Member Author

sciunto commented Oct 9, 2018

Good point. I open another issue for that. I think that ordering our examples would be useful beyond this PR (eg operation on numpy arrays could come among the first ones).

@emmanuelle
Copy link
Member

Merging, thanks!

@emmanuelle emmanuelle merged commit 9cb7cd3 into scikit-image:master Oct 9, 2018
@sciunto sciunto deleted the doc_data branch October 9, 2018 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants