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 morphology module #6814

Merged
merged 11 commits into from Sep 25, 2023

Conversation

decorouz
Copy link
Contributor

@decorouz decorouz commented Mar 10, 2023

Description

#6761 tasks:
Added summary description docstrings to skimage.morphology.

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 @decorouz, this is a great start. Though, in #6761 we are recommending to pick only one module of the list so that other applicants have the chance to do so as well. E.g. #6799 was already created for the measure module. Could you update this pull request so that it only updates a module that's not already worked on?

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

decorouz commented Mar 13, 2023

Hey @decorouz, this is a great start. Though, in #6761 we are recommending to pick only one module of the list so that other applicants have the chance to do so as well. E.g. #6799 was already created for the measure module. Could you update this pull request so that it only updates a module that's not already worked on?

Hi @lagru , I have updated the PR. Please review.

skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
INSTALL.rst Outdated
@@ -339,7 +339,7 @@ before you get started.
conda install -c conda-forge --file requirements/test.txt
conda install -c conda-forge pre-commit
# Install build dependencies of scikit-image
conda install -c conda-forge --file requirements/build.txt
pip install -r requirements/build.txt
Copy link
Member

Choose a reason for hiding this comment

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

For some reason GitHub shows three other commits that have been merged as part of this pull request (see https://github.com/scikit-image/scikit-image/pull/6814/commits). I'm confused why they show up in the diff for this PR, because they are already merged. 🤔

@decorouz, I can check out your branch locally and remove them for you if you want, but let me know if you want to give it a shot yourself and I'll try to help with that. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would like to give it a shot myself.
I looked up stack-overflow for some pointers.

git checkout <branch>

git revert <commits> 

git push origin <branch> --force

Would this address the issue?

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably do it with the git rebase -i HEAD~9 and then git push -f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lagru
Following your advice, I was able to remove the confusing commits.
I also updated the docstring to match numpydoc style closely.
Please review.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think removing the commits worked. 🤔 At least they still showed up on GitHub and when I checked you branch locally. I've removed them now. You should be able to pull these changes to your local branch with the following:

git branch add_description_skimage-backup
git fetch --all
git reset --hard origin/add_description_skimage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @lagru.
That was some commits clean-up-job you did on my behalf.

lagru
lagru previously approved these changes Mar 16, 2023
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! 🎉 Thank you @decorouz.

@lagru lagru dismissed their stale review March 16, 2023 22:11

Outdated

@lagru lagru changed the title Add description skimage Add docstring to the morphology module Mar 16, 2023
Co-authored-by: Lars Grüter <lagru+github@mailbox.org>
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, @decorouz! I appreciate your effort at writing descriptive sentences.

@@ -1,3 +1,11 @@
"""Utilities that process images based on shapes.
Copy link
Member

Choose a reason for hiding this comment

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

I find there's a slight wording issue here, because it sounds as if images had to be "based on shapes" when, really, what is intended is that the actual processing is based on shapes (geometry).

Copy link
Contributor Author

@decorouz decorouz Mar 22, 2023

Choose a reason for hiding this comment

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

Valid point. Thank you.
I will improve the wordings.

skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
decorouz and others added 3 commits March 22, 2023 12:21
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@lagru lagru self-requested a review March 27, 2023 16:09
@github-actions
Copy link

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 24, 2023
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.

I suggested some changes, which I think read better. I must acknowledge the value of the 'dormant' bot here, because this would have fallen out of my radar...

skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
skimage/morphology/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
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, @decorouz!

@soupault soupault merged commit b1781c8 into scikit-image:main Sep 25, 2023
20 checks passed
@stefanv stefanv added this to the 0.22 milestone Sep 25, 2023
@soupault
Copy link
Member

Thank you for the contribution @decorouz !

@lagru lagru removed the 😴 Dormant no recent activity label Sep 27, 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 👋 Outreachy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants