Improve docstring for rolling_ball function. by mkcor · Pull Request #7682 · scikit-image/scikit-image

mkcor · 2025-01-29T18:13:12Z

Description

As I started tackling #7680, I had to refresh my memory and remember what the rolling-ball algorithm was about. I found there was room for improvement with the function's docstring, in spite of the multiple reviews back in the days! This PR is an attempt at providing clearer descriptions. Then I'll go through the gallery example.

Checklist

A descriptive but concise pull request title
Docstrings for all functions
Unit tests
A gallery example in ./doc/examples for new features
Contribution guide is followed

Release note

For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.

Improve docstring of `skimage.restoration.rolling_ball`.

mkcor · 2025-01-29T18:14:26Z

skimage/restoration/_rolling_ball.py

    kernel : ndarray, optional
-        The kernel to be rolled/translated in the image. It must have the
-        same number of dimensions as ``image``. Kernel is filled with the
-        intensity of the kernel at that position.


@FirefoxMetzger I originally tried fixing the sentence "Kernel is filled with the intensity of the kernel" but then I thought these details would fit better in the Notes.

stefanv

Thanks @mkcor!

I started reading the docstring, and was immediately confused by things you explain later in the notes. I think it would be helpful to give a short, intuitive explanation as part of the function description (it doesn't require that much!). But also fine to add a forward reference to the notes, if you prefer.

skimage/restoration/_rolling_ball.py

stefanv · 2025-01-29T21:45:49Z

skimage/restoration/_rolling_ball.py

+    This function estimates the background intensity of an n-dimensional
+    image. Typically, it is useful for background subtraction in case of
+    uneven exposure. It is a generalization of the well-known rolling-ball
+    algorithm [1]_.


Suggested change

This function estimates the background intensity of an n-dimensional

image. Typically, it is useful for background subtraction in case of

uneven exposure. It is a generalization of the well-known rolling-ball

algorithm [1]_.

This function is a generalization of the rolling-ball algorithm [1]_

to estimate the background intensity of an N-dimensional

image. This is useful for background subtraction in case of

uneven exposure. Think of the image as a landscape

(where altitude is determined by intensity), under which

a ball of given radius is rolled. At each position, the ball's

apex determines the resulting background intensity.

@stefanv I included this in 13eeb19. I wrote "n-dimensional" instead of "N-dimensional" although we use both conventions throughout the codebase; in this case, I think it's better to differentiate it from the "N" used in the Notes when referring to the algorithm's polynomial complexity.

No problem; we should strive to get it uniform eventually, either n/N fine.

skimage/restoration/_rolling_ball.py

Co-authored-by: Stefan van der Walt <[email protected]>

stefanv · 2025-01-29T22:56:53Z

Totally fine to hyphenate however you see fit, by the way!

stefanv · 2025-01-30T00:15:06Z

Sorry @mkcor, I had to cancel your CI checks on macos because I need the runners. But, I will have that situation under control soon. Happy to merge your changes when you're ready (wasn't sure whether you had addressed my comments or not 😅).

stefanv · 2025-01-30T01:22:39Z

(Added new GH runners and restarted tests; but the rolling ball is the culprit in making them so slow!)

mkcor · 2025-01-30T08:09:32Z

wasn't sure whether you had addressed my comments or not 😅

Not entirely yet! 🙏

…mage into improve-rolling-docstring

Co-authored-by: Stefan van der Walt <[email protected]>

mkcor · 2025-01-30T19:29:57Z

I used the term 'doctest' in a commit message although no check is performed in these docstring examples. They just illustrate how the function may be used.

Btw, this is now ready for final review!

stefanv · 2025-01-29T22:57:21Z

skimage/restoration/_rolling_ball.py


 def rolling_ball(image, *, radius=100, kernel=None, nansafe=False, num_threads=None):
-    """Estimate background intensity by rolling/translating a kernel.
+    """Estimate background intensity using the rolling ball algorithm.


Suggested change

"""Estimate background intensity using the rolling ball algorithm.

"""Estimate background intensity using the rolling-ball algorithm.

As long as its consistent :)

The gallery example will soon be consistent :)

stefanv · 2025-01-30T19:30:06Z

skimage/restoration/_rolling_ball.py

+    This function estimates the background intensity of an n-dimensional
+    image. Typically, it is useful for background subtraction in case of
+    uneven exposure. It is a generalization of the well-known rolling-ball
+    algorithm [1]_.


No problem; we should strive to get it uniform eventually, either n/N fine.

stefanv · 2025-01-30T19:33:16Z

Looks good! The failure is related, however.

…ailure * origin/main: Add zizmor to pre-commit; address GH workflow issues raised (scikit-image#7662) Reenable test marked as flaky on Azure. (scikit-image#7694) Simultaneously resolve all dependencies; add pip caching (scikit-image#7690) Copy keypoints if necessary to preserve contiguity (scikit-image#7692) CI cleanup (scikit-image#7693) Port CI testing from Azure to GitHub (scikit-image#7687) Lower CI build verbosity Use pytest config in pyproject.toml in CI (scikit-image#7555) Improve docstring for rolling_ball function. (scikit-image#7682)

Improve docstring for rolling_ball function

f077624

mkcor commented Jan 29, 2025

View reviewed changes

jarrodmillman added the 📄 type: Documentation Updates, fixes and additions to documentation label Jan 29, 2025

stefanv reviewed Jan 29, 2025

View reviewed changes

Apply suggestions from code review

d4ae76a

Co-authored-by: Stefan van der Walt <[email protected]>

Merge branch 'main' into improve-rolling-docstring

26e4920

mkcor and others added 4 commits January 30, 2025 15:32

Use hyphenated form

43b4f3b

Update doctests with new import convention

eabbbc2

Merge branch 'improve-rolling-docstring' of github.com:mkcor/scikit-i…

04f7231

…mage into improve-rolling-docstring

Reorganize docstring notes

13eeb19

Co-authored-by: Stefan van der Walt <[email protected]>

stefanv approved these changes Jan 30, 2025

View reviewed changes

Fix doctests

b3ced63

jarrodmillman approved these changes Jan 30, 2025

View reviewed changes

jarrodmillman merged commit c473ca0 into scikit-image:main Jan 30, 2025
25 checks passed

mkcor deleted the improve-rolling-docstring branch January 30, 2025 21:55

stefanv added this to the 0.25.2 milestone Feb 4, 2025

mkcor mentioned this pull request Feb 18, 2025

Cut down runtime of rolling-ball gallery example. #7705

Merged

1 task

-    This function estimates the background intensity of an n-dimensional
-    image. Typically, it is useful for background subtraction in case of
-    uneven exposure. It is a generalization of the well-known rolling-ball
-    algorithm [1]_.
+    This function is a generalization of the rolling-ball algorithm [1]_
+    to estimate the background intensity of an N-dimensional
+    image. This is useful for background subtraction in case of
+    uneven exposure. Think of the image as a landscape
+    (where altitude is determined by intensity), under which
+    a ball of given radius is rolled. At each position, the ball's
+    apex determines the resulting background intensity.

	"""Estimate background intensity using the rolling ball algorithm.
	"""Estimate background intensity using the rolling-ball algorithm.

Uh oh!

Conversation

mkcor commented Jan 29, 2025 • edited by lagru Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Release note

Uh oh!

mkcor Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

stefanv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stefanv Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

mkcor Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

stefanv Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stefanv commented Jan 29, 2025

Uh oh!

stefanv commented Jan 30, 2025

Uh oh!

stefanv commented Jan 30, 2025

Uh oh!

mkcor commented Jan 30, 2025

Uh oh!

mkcor commented Jan 30, 2025

Uh oh!

stefanv Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

mkcor Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

stefanv Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

stefanv commented Jan 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mkcor commented Jan 29, 2025 •

edited by lagru

Loading