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 docstring to the transform module #6797

Merged
merged 16 commits into from Apr 4, 2023
Merged

Conversation

rum1887
Copy link
Contributor

@rum1887 rum1887 commented Mar 9, 2023

Description

Added Docstring to the transform module. Updates the issue #6761

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

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.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

@rum1887 rum1887 changed the title Added Docstrings for the transform module. Updates the issue #6761 Added Docstrings for the transform module. Mar 9, 2023
@lagru lagru added the 📄 type: Documentation Updates, fixes and additions to documentation label Mar 9, 2023
skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
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.

Wow, nice work @rum1887! That's a lot more detailed than what I would have anticipated and already reads quite well. We could include this as is, however with how detailed it is I fear it might get outdated quickly.

What do you think about restructuring things a bit to have a more natural flow? Maybe something like the following that includes what you wrote but in a more general style that's less focused on the implementation and more on the purpose.

"This module includes tools to transform images and volumetric data such as:

  • Geometric transformations can manipulate the shape and position of an image and are useful for tasks such as image registration, alignment, and geometric correction.
  • ...

"

@rum1887
Copy link
Contributor Author

rum1887 commented Mar 12, 2023

Wow, nice work @rum1887! That's a lot more detailed than what I would have anticipated and already reads quite well. We could include this as is, however with how detailed it is I fear it might get outdated quickly.

What do you think about restructuring things a bit to have a more natural flow? Maybe something like the following that includes what you wrote but in a more general style that's less focused on the implementation and more on the purpose.

"This module includes tools to transform images and volumetric data such as:

  • Geometric transformations can manipulate the shape and position of an image and are useful for tasks such as image registration, alignment, and geometric correction.
  • ...

"

I agree that it might get outdated :)

How about providing a few functions as examples under each category for ease of understanding instead of an exhaustive list of functions available at scikit-image.tranform module?

Maybe I could add a note mentioning that the examples are not exhaustive and point them to the documentation

@rum1887 rum1887 changed the title Added Docstrings for the transform module. Added Docstrings to the transform module. Mar 12, 2023
@rum1887 rum1887 changed the title Added Docstrings to the transform module. Added a Docstring to the transform module. Mar 12, 2023
skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
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.

@rum1887, thanks. This is shaping up! :) I've left a few more suggestions.

skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
@rum1887
Copy link
Contributor Author

rum1887 commented Mar 15, 2023

@rum1887, thanks. This is shaping up! :) I've left a few more suggestions.

@lagru done :)

@rum1887
Copy link
Contributor Author

rum1887 commented Mar 16, 2023

@lagru Do review this :)

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 great and almost ready! Except for the comment on capitalization I'm happy to approve.

skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
skimage/transform/__init__.py Outdated Show resolved Hide resolved
@lagru lagru changed the title Added a Docstring to the transform module. Add docstring to the skimage.transform module Mar 16, 2023
@lagru lagru changed the title Add docstring to the skimage.transform module Add docstring to the transform module Mar 16, 2023
@rum1887
Copy link
Contributor Author

rum1887 commented Mar 21, 2023

Looks great and almost ready! Except for the comment on capitalization I'm happy to approve.

Done :)

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.

Agreed, done. :) Thank you @rum1887!

@lagru
Copy link
Member

lagru commented Mar 27, 2023

Hey @mkcor, taking the liberty to ping you here. I think this can be merged or is nearly done depending on your review. 😉

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.

Very nice docstring!! Thanks, @rum1887. I just have a pending question, because I was surprised on a formal level...

@lagru thanks for pinging me!

These transforms change the appearance of an image without changing its
content. They are useful for tasks such a creating image mosaics,
applying artistic effects, and visualizing image data.
Examples: :func:`~skimage.transform.warp`, inverse transforms.
Copy link
Member

Choose a reason for hiding this comment

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

Usually (see all the rest above), what's under 'Examples' are specific (linked to) functions or classes... Why are 'inverse transforms' in the wild like this? 😉

Copy link
Member

Choose a reason for hiding this comment

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

Probably because there are multiple iradon* function's to link to and it wasn't clear which one. I would probably just link iradon as a well-known example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably because there are multiple iradon* function's to link to and it wasn't clear which one. I would probably just link iradon as a well-known example.

Usually (see all the rest above), what's under 'Examples' are specific (linked to) functions or classes... Why are 'inverse transforms' in the wild like this? 😉

yeah , was not sure which inverse transform to link to , so didn't link to keep it more general.
Since these are just examples , linking to a well known function would also work.

Thanks a lot for your review , learnt a lot and had a great time working on this !

rum1887 and others added 2 commits March 28, 2023 19:46
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@lagru lagru merged commit 70c3fd8 into scikit-image:main Apr 4, 2023
20 checks passed
@lagru
Copy link
Member

lagru commented Apr 4, 2023

Thanks @rum1887!

@jarrodmillman jarrodmillman added this to the 0.21 milestone Apr 4, 2023
@rum1887 rum1887 deleted the add-docstring branch April 5, 2023 09:28
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 👋 Outreachy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants