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 dosctrings to test_adapt_rgb.py #6776

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

harshithakolipaka
Copy link
Contributor

Description

Updates the test_adapt_rgb.py file. This updates 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.

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.

Hey @harshithakolipaka, welcome and great to see you improving our documentation! I've left a few suggestions. Please feel encouraged to ask if something is not clear to you. 🙂

I am a bit surprised that you chose to add docstrings to our test suite. Files and functions that start with test_ are usually part of our test suite and not of our official API. It's not a bad thing to add them here, however, it's also not strictly necessary. 😉

However, I think it would be useful to add these docstrings to our "Adapting gray-scale filters to RGB images" tutorial. That tutorial uses adapt_rgb in a similar manner.

You are referencing #6761. Perhaps the description of that issue is a bit confusing. The goal of #6761 is not to add docstrings to every function anywhere, but to add it to each submodule. In Python that means, adding a docstring at the beginning of each __init__.py file. You can see an example of this here

"""Utilities to read and write images in various formats.
The following plug-ins are available:
"""

skimage/color/tests/test_adapt_rgb.py Outdated Show resolved Hide resolved
Comment on lines 63 to 80
"""
Applies a binary mask to an RGB image, with each color channel processed
independently.

Pixels where `mask` is True will be set to zero.

Parameters
----------
image : ndarray
Input RGB image.
mask : ndarray, bool
Boolean mask to apply to the image.

Returns
-------
result : ndarray
Copy of the input image with masked pixels set to zero.
"""
Copy link
Member

Choose a reason for hiding this comment

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

This docstring already looks quite close to the numpydoc style guide that we follow in scikit-image. There are a few things I would change:

  • Remove the indent before """ in the first line
  • Shorten the summary at the beginning to a single line and use the imperative mood (as if you where telling code what to do). For example, "Mask each color channel of an RGB image independently".
  • Change mask : ndarray, bool to mask : ndarray.

You could also try updating the other docstrings accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure , i will keep these points in mind and update accordingly

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. :) I see that you added some of the changes but I think you missed a few things. For example, in edges_hsv_uint and you could try to shorten the summaries to one line (see my third point above).

Apart from that it looks perfect!

@lagru lagru changed the title Update test_adapt_rgb.py Add dosctrings to test_adapt_rgb.py Mar 5, 2023
@lagru
Copy link
Member

lagru commented Mar 5, 2023

I also took the liberty to update the PR title to something a bit more concise. 😉

@lagru lagru added the 📄 type: Documentation Updates, fixes and additions to documentation label Mar 5, 2023
@harshithakolipaka
Copy link
Contributor Author

Hello @lagru, I have made the necessary changes suggested to this file and also created a new PR for the skimage.color sub module.

Comment on lines +123 to +125
"""
Applies Sobel edge detection to the value channel of an HSV converted RGB
image and output it in 16-bit integer format.
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
"""
Applies Sobel edge detection to the value channel of an HSV converted RGB
image and output it in 16-bit integer format.
"""Apply Sobel filter to the value channel of an HSV-converted RGB image.
Return the output in 16-bit unsigned integer format.

Does that makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

Hey @harshithakolipaka, do you agree with this suggestion? :)

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Hey, there hasn't been any activity on this pull request for more than 180 days. For now, we have marked it as "dormant" until there is some new activity. You are welcome to reach out to people by mentioning them here or on our forum if you need more feedback! Otherwise, we would be thankful for a short update, for example if you would like to continue or if you are okay with someone else doing so. In any case, thank you for your contributions so far!

@github-actions github-actions bot added the 😴 Dormant no recent activity label Sep 6, 2023
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 😴 Dormant no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants