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 stitching gallery example #5365

Merged
merged 34 commits into from
Sep 7, 2021
Merged

Conversation

rfezzani
Copy link
Member

Description

This gallery example is a show case for the stitching problem using skimage. It is based on my answer to a question in image.sc forum and its related issue #5330.

I used the moon image to simulate a set of noisy and tilted images:
input

And here is the obtained composite image:
compoist

I will appreciate any help for rephrasing and clarifying the comments 😉.

Checklist

For reviewers

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

@pep8speaks
Copy link

pep8speaks commented Apr 30, 2021

Hello @rfezzani! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 94:16: E126 continuation line over-indented for hanging indent

Comment last updated at 2021-09-03 07:34:33 UTC

@rfezzani rfezzani added the 📄 type: Documentation Updates, fixes and additions to documentation label Apr 30, 2021
Copy link
Member

@alexdesiqueira alexdesiqueira left a comment

Choose a reason for hiding this comment

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

Looks awesome @rfezzani, thank you for this! I'm just leaving some comments on consistency.

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
@rfezzani
Copy link
Member Author

rfezzani commented May 3, 2021

Thank you @alexdesiqueira for your review 😉

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks, this is a nice addition. I have requested a number of small edits to the text

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
rfezzani and others added 10 commits May 3, 2021 22:45
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
Co-authored-by: Gregory R. Lee <grlee77@gmail.com>
@rfezzani
Copy link
Member Author

rfezzani commented May 3, 2021

thank you @grlee77 for your review 😉

Copy link
Member

@lagru lagru 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 so far! I didn't look to deep yet so I only found nitpicks and maybe a small oversight regarding the noise generation.

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
@rfezzani
Copy link
Member Author

rfezzani commented May 6, 2021

Thank you @lagru for your review 😉

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
rfezzani and others added 3 commits June 7, 2021 09:30
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
rfezzani and others added 4 commits June 7, 2021 09:30
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@rfezzani
Copy link
Member Author

rfezzani commented Jun 7, 2021

Thank you @mkcor for your review 😉 . Do you request any other change @grlee77?

@mkcor
Copy link
Member

mkcor commented Jun 7, 2021

Thank you @mkcor for your review wink . Do you request any other change @grlee77?

You're welcome, @rfezzani! I have two qualitative suggestions to add:

  • I prefer titles at the imperative ("Adjust gamma and log contrast" rather than "Gamma and log contrast adjustment") because they are more engaging and they make it clear the reader is embarking on a tutorial; I'm well aware that, as of today, the gallery isn't standardized in this respect, but I've been pushing in this direction whenever a new tutorial-like example was submitted;
  • I find it's more pedagogic to import submodules rather than functions directly, so that when the reader runs or reads the code, they see feature.corner_harris, filters.gaussian, etc. and they think "Gaussian is a filter indeed" and, indirectly, they learn the codebase organization and, also, the import header is then more compact.

@mkcor
Copy link
Member

mkcor commented Jun 7, 2021

@rfezzani
Copy link
Member Author

rfezzani commented Jun 7, 2021

PS: @rfezzani did you know you could apply suggested changes as a batch? https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

Nop, I didn't 🙂 thank you for the tip...

@rfezzani
Copy link
Member Author

@mkcor, I implemented submodules imports instead of functions import as you suggested. If you have any idea for an imperative title I would be happy to modify the actual title 😉.

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
@mkcor
Copy link
Member

mkcor commented Aug 27, 2021

@mkcor, I implemented submodules imports instead of functions import as you suggested. If you have any idea for an imperative title I would be happy to modify the actual title wink.

Wonderful! I would say "Reconstruct images with simple image stitching" or maybe "Recompose images ..." (likewise, I would suggest "Reconstruct an image with inpainting" here https://scikit-image.org/docs/stable/auto_examples/filters/plot_inpaint.html).

rfezzani and others added 2 commits September 1, 2021 10:44
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@rfezzani
Copy link
Member Author

rfezzani commented Sep 1, 2021

@mkcor, I modified the title to Assemble images with simple image stitching, is it OK for you?

Copy link
Contributor

@grlee77 grlee77 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 to me. I just left one (optional) suggestion related to variable names.

doc/examples/registration/plot_stitching.py Outdated Show resolved Hide resolved
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Thank you for your patience, @rfezzani! I left two suggestions to include hyperlinks, but if they are confusing for now, I can merge today and we can dig deeper another time.

#
# .. note::
# This step is performed using the approach described in the
# **Robust matching using Ransac** gallery example, but any other
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# **Robust matching using Ransac** gallery example, but any other
# :ref:`sphx_glr_auto_examples_transform_plot_matching.py` gallery example, but any other

# .. note::
# This step is performed using the approach described in the
# **Robust matching using Ransac** gallery example, but any other
# method from the **Image registration** section can be applied,
Copy link
Member

Choose a reason for hiding this comment

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

Here I'm not sure how the Sphinx link to the #image-registration anchor should be done... Maybe

Suggested change
# method from the **Image registration** section can be applied,
# method from the :ref:`sphx_glr_auto_examples_registration` section can be applied,

?

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 confess that I don't really know how to properly manage Sphinx hyperlinks (that's why I didn't included them 😅)

Copy link
Member

Choose a reason for hiding this comment

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

All good, I'll take a look locally when I get a chance!

@mkcor mkcor merged commit 66979f2 into scikit-image:main Sep 7, 2021
@rfezzani
Copy link
Member Author

rfezzani commented Sep 7, 2021

🎉 Thank you all for your help and review @alexdesiqueira @grlee77 @lagru @mkcor

@alexdesiqueira
Copy link
Member

Thank you @rfezzani!

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

6 participants