Fix invalid syntax (M, N) ndarray: docstub fixes (III).#8014
Fix invalid syntax (M, N) ndarray: docstub fixes (III).#8014mkcor merged 41 commits intoscikit-image:mainfrom
Conversation
|
Just fixed another group of errors: 164be40. |
| image : ndarray of shape (M, N) and dtype (uint8 or uint16) | ||
| Input image. | ||
| footprint : 2-D array | ||
| footprint : ndarray |
There was a problem hiding this comment.
I tried passing a list of list and got an error.
There was a problem hiding this comment.
Same with a list of tuples. I get the following:
ValueError: All elements of footprint sequence must be a 2-tuple where the first element of the tuple is an ndarray and the second is an integer indicating the number of iterations.
which, I admit, is not crystal clear to me... It's easier to just pass/request an ndarray! 😩
There was a problem hiding this comment.
You might be confusing array with array-like? Only the latter would accept something coercible like a list of lists.
That said, happy to use ndarray here. Since we don't really accept anything besides numpy arrays anyway.
There was a problem hiding this comment.
Yes, you mean that, potentially, array could be an xarray.DataArray, etc. My thinking:
- We have an overall note warning that we don't support non-NumPy arrays.
- I suspect that the author(s) of these docstrings didn't necessarily put that much thought into it when they wrote
array(maybe they even meantarray_like?).
That's why I don't take their word for it, I double check and propose a change accordingly.
Btw I just found out that array_like is an official term! With an official spelling then. I'll use it then.
There was a problem hiding this comment.
Well, a structuring element should be smaller than the input image, otherwise it doesn't really mean anything... We could provide a tutorial about using structuring elements? I find that's a more general topic.
| Parameters | ||
| ---------- | ||
| image : 2-D array (uint8, uint16) | ||
| image : ndarray of shape (M, N) and dtype (uint8 or uint16) |
There was a problem hiding this comment.
@lagru maybe you'd prefer leaving just ... and dtype int?
There was a problem hiding this comment.
I think we need to figure out if type checkers view np.uint8 as compatible with np.uint16. That would be useful since the latter can hold the former without information loss.
Then – for input – we could just use uint16.
Again, we can easily come up with our own union type uint16-like = uint8 | uint16. But than the question is if that doesn't decrease readability too much for our taste.
There was a problem hiding this comment.
#8011 has several instances of dtype (uint8 or uint16) so... shipping anyway, in the name of 'good enough for now?' Taking note of this question for later discussion?
There was a problem hiding this comment.
Can we resolve the issue around brackets? This currently reads very oddly "and dtype (...)". If it's discussed elsewhere, I'll probably run into it soon during review :)
There was a problem hiding this comment.
For now I would suggest sticking with a 'lower common denominator,' i.e., ... and dtype int.
| Parameters | ||
| ---------- | ||
| image : ndarray of dtype (uint8 or uint16) | ||
| image : ndarray of dtype (int or float) |
src/skimage/filters/_median.py
Outdated
| with the same number of dimensions as `image`. | ||
| If None, `footprint` will be a N-D array with 3 elements for each | ||
| dimension (e.g., vector, square, cube, etc.). | ||
| out : ndarray, same dtype as input `image`, optional |
There was a problem hiding this comment.
| out : ndarray, same dtype as input `image`, optional | |
| out : ndarray of dtype int, optional |
There was a problem hiding this comment.
Oh, no! I'm giving up... ski.filters.median and ski.filters.rank.median are not the same! 😱
The former does return an output image of same dtype as the input image. The latter does not.
There was a problem hiding this comment.
Ha, there we go! The AI agent got it:
24-24: Clarify dtype when behavior='rank'.
The docstring currently states the output dtype always matches the input, but skimage.filters.rank.median does not guarantee that. Consider qualifying the dtype by behavior to avoid misleading docs.
📝 WalkthroughWalkthroughStandardizes docstring parameter/return wording across many modules (e.g., "(M, N) ndarray" → "ndarray of shape (M, N)") and updates the typing workflow ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/skimage/measure/pnpoly.py (1)
21-37: Fix typo in label legend (0vsO).The label list uses the letter O for outside; it should be the digit 0 to match the numeric labels described elsewhere.
✏️ Proposed docstring fix
- O - outside, 1 - inside, 2 - vertex, 3 - edge. + 0 - outside, 1 - inside, 2 - vertex, 3 - edge. ... - O - outside, 1 - inside, 2 - vertex, 3 - edge. + 0 - outside, 1 - inside, 2 - vertex, 3 - edge.src/skimage/measure/_colocalization.py (1)
188-196: Remove stray symbol in docstring.
The “♣” character appears to be accidental and will render oddly in docs.✅ Proposed fix
- Must have ♣same dimensions as `image0`. + Must have same dimensions as `image0`.
🤖 Fix all issues with AI agents
In `@src/skimage/filters/_median.py`:
- Around line 26-28: The docstring for the `mode` parameter in
src/skimage/filters/_median.py contains a curly left single quote in the list
element "'‘wrap'" which triggers RUF002 and renders incorrectly; edit the `mode`
parameter description (the line showing mode : {'reflect', 'constant',
'nearest', 'mirror','‘wrap'}) to use a normal ASCII single quote and proper
spacing so it reads {'reflect', 'constant', 'nearest', 'mirror', 'wrap'} (i.e.
replace the curly quote with a straight quote and add the missing space after
the comma).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/typing.ymlsrc/skimage/draw/_polygon2mask.pysrc/skimage/feature/_hog.pysrc/skimage/feature/corner.pysrc/skimage/filters/_median.pysrc/skimage/filters/lpi_filter.pysrc/skimage/filters/rank/_percentile.pysrc/skimage/filters/rank/generic.pysrc/skimage/filters/thresholding.pysrc/skimage/measure/_colocalization.pysrc/skimage/measure/_regionprops_utils.pysrc/skimage/measure/entropy.pysrc/skimage/measure/pnpoly.pysrc/skimage/morphology/convex_hull.pysrc/skimage/restoration/deconvolution.pysrc/skimage/segmentation/_chan_vese.pysrc/skimage/segmentation/_felzenszwalb.pysrc/skimage/segmentation/_quickshift.pysrc/skimage/transform/_geometric.pysrc/skimage/transform/_thin_plate_splines.pysrc/skimage/transform/hough_transform.py
🧰 Additional context used
🪛 Ruff (0.14.11)
src/skimage/filters/_median.py
26-26: Docstring contains ambiguous ‘ (LEFT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: macos-latest-cp3.13
- GitHub Check: macos-latest-cp3.11
- GitHub Check: macos-15-intel-cp3.13
- GitHub Check: windows-cp3.12-pre
- GitHub Check: windows-cp3.11-optional-deps
- GitHub Check: windows-cp3.13-default
- GitHub Check: macos-docs
- GitHub Check: linux-cp3.12-install-from-sdist
- GitHub Check: windows-cp3.11-default
- GitHub Check: pyodide0.29.0-cp3.13-emscr4.0.9
- GitHub Check: linux-cp3.14-default
- GitHub Check: linux-cp3.12-pre
- GitHub Check: linux-cp3.11-mini-req-eager-import
- GitHub Check: linux-cp3.11-mini-req-optional-deps
- GitHub Check: linux-cp3.13-default
- GitHub Check: linux-cp3.12-optimize and no pooch
- GitHub Check: linux-cp3.12-optional-deps
- GitHub Check: linux-cp3.11-default
- GitHub Check: linux-cp313t-default
🔇 Additional comments (42)
src/skimage/feature/corner.py (9)
551-552: Docstring shape wording looks good.
Consistent “ndarray of shape (M, N)” phrasing improves clarity.
616-617: Docstring update is clear and consistent.
Matches the standardized shape notation.
673-674: Good docstring normalization.
The updated type/shape format is consistent.
749-750: Docstring shape annotation looks correct.
No issues with this change.
812-813: Consistent docstring wording.
The “ndarray of shape (M, N)” format reads well.
885-886: Docstring update approved.
Standardized shape notation is clearer.
957-958: Docstring shape format is consistent.
Nice alignment with the new convention.
1147-1148: Docstring clarification looks good.
No concerns with the new shape phrasing.
1249-1250: Docstring change LGTM.
Shape notation is now consistent with the rest of the module.src/skimage/feature/_hog.py (1)
29-30: LGTM — docstring shape standardization looks consistent.src/skimage/segmentation/_felzenszwalb.py (1)
44-45: LGTM — return type wording is clearer and consistent.src/skimage/segmentation/_chan_vese.py (1)
203-204: LGTM — parameter shape wording is consistent.src/skimage/segmentation/_quickshift.py (1)
29-61: LGTM — docstring shape/dtype clarification is helpful.src/skimage/measure/entropy.py (1)
13-13: LGTM!The docstring format change to
ndarray of shape (M, N)aligns with the PR's standardization effort and follows NumPy docstring conventions..github/workflows/typing.yml (1)
50-51: LGTM!The reduction in
--allow-errorsfrom 1038 to 951 reflects the docstring fixes in this PR. The--workers -1flag enables parallel processing using all available cores, which should speed up the docstub generation.src/skimage/measure/_regionprops_utils.py (2)
422-422: LGTM!The docstring update for
perimeteris consistent with the PR's standardization and matches the function's 2D image requirement (enforced at line 452-453).
489-489: LGTM!The docstring update for
perimeter_croftonfollows the same standardization pattern and aligns with the 2D constraint check at line 533-534.src/skimage/filters/thresholding.py (2)
47-47: LGTM!The docstring update for the internal
_try_allfunction follows the standardized format.
117-117: LGTM!The docstring update for
try_all_thresholdis consistent with the PR's documentation standardization effort.src/skimage/restoration/deconvolution.py (2)
155-155: LGTM!The docstring update for the
imageparameter follows the standardized format.
199-199: LGTM!The docstring update for the
x_postmeanreturn value maintains consistency with the input parameter format and the PR's standardization goal.src/skimage/draw/_polygon2mask.py (1)
13-15: Docstring clarification looks good.
The shape/coordinate detail is clearer and consistent with the rest of the API docs.src/skimage/filters/lpi_filter.py (4)
123-123: Docstring shape normalization is clear.
140-141: Consistent parameter shape description.
180-181: Docstring shape update looks good.
225-226: Docstring shape update looks good.src/skimage/measure/_colocalization.py (2)
20-23: Docstring shape update is clear.
105-106: Docstring shape update is clear.src/skimage/morphology/convex_hull.py (2)
27-30: Docstring shape update is clear.
181-182: Docstring shape update is clear.src/skimage/filters/_median.py (1)
19-24: Docstring wording updates look good.
Clarity and consistency are improved without changing behavior.Also applies to: 32-49, 52-65
src/skimage/filters/rank/_percentile.py (1)
119-144: Docstring shape/dtype standardization looks good.The updated “ndarray of shape” phrasing is consistent and clear.
Also applies to: 170-189
src/skimage/filters/rank/generic.py (1)
1697-1723: Docstring and example refresh looks solid.The clarified dtype wording and the updated example usage are consistent with the rest of the module.
src/skimage/transform/hough_transform.py (1)
131-132: Docstring shape annotations are consistent.Clear and aligned with the standardized “ndarray of shape” style.
Also applies to: 198-199
src/skimage/transform/_thin_plate_splines.py (1)
18-19: Docstring shape clarifications look good.Inputs/outputs are now described consistently across methods.
Also applies to: 106-113, 141-144, 237-240
src/skimage/transform/_geometric.py (7)
265-271: Abstract interface docstring update is clear.The standardized shape wording improves consistency.
546-552: FundamentalMatrixTransform docstring updates look consistent.The shape annotations are now explicit and uniform across methods.
Also applies to: 574-588, 629-632, 710-713
869-872: EssentialMatrixTransform docstring updates are clear.Consistent “array_like of shape” wording across estimation APIs.
Also applies to: 923-926
1128-1133: ProjectiveTransform parameter shape updates look good.The updates align with the standardized docstring style.
Also applies to: 1311-1316
1998-2001: EuclideanTransform docstring updates are consistent.No issues with the updated parameter shape descriptions.
Also applies to: 2059-2062
2375-2382: PolynomialTransform docstring updates are clear and consistent.The input/output shape descriptions are now explicit.
Also applies to: 2447-2453
2681-2689: matrix_transform docstring update looks good.Clearer parameter/return shape descriptions.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| image1 : ndarray of shape (M, N) | ||
| Image of channel B. All pixel values should be non-negative. | ||
| Must have same dimensions as `image0` | ||
| Must have same dimensions as `image0`. |
There was a problem hiding this comment.
The AI agent brought my attention to this code block. I guess we could remove this sentence, since we specify that both image0 and image1 are ndarray's of shape (M, N).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/skimage/filters/_median.py`:
- Around line 38-42: Fix the docstring wording around the version note and the
behavior parameter: change the fragment "`cval` was added in 0.15 is used when
``behavior='ndimage'``" to "`cval` was added in 0.15 and is used when
``behavior='ndimage'``", and reword the `behavior` description to something like
"behavior : {'ndimage', 'rank'}, optional — Choose 'ndimage' (new behavior) or
'rank' (old behavior from scikit-image < 0.15)." Keep the references to the call
targets intact (i.e., :func:`skimage.filters.rank.median` for the old behavior
and :func:`scipy.ndimage.median_filter` for the new behavior) and ensure the
symbols `cval` and `behavior` are mentioned exactly as in the docstring.
♻️ Duplicate comments (1)
src/skimage/filters/_median.py (1)
24-24: Clarify dtype whenbehavior='rank'.
The docstring currently states the output dtype always matches the input, butskimage.filters.rank.mediandoes not guarantee that. Consider qualifying the dtype by behavior to avoid misleading docs.📝 Suggested docstring tweak
- out : ndarray, same dtype as input `image`, optional + out : ndarray, optional + Output array. For ``behavior='ndimage'``, dtype matches input. For + ``behavior='rank'``, dtype follows :func:`skimage.filters.rank.median`. @@ - out : ndarray, same dtype as input `image` + out : ndarray + Output image. For ``behavior='ndimage'``, dtype matches input. For + ``behavior='rank'``, dtype follows :func:`skimage.filters.rank.median`.Also applies to: 52-52
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/skimage/filters/_median.pysrc/skimage/measure/_colocalization.pysrc/skimage/measure/pnpoly.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: linux-cp3.13-default
- GitHub Check: linux-cp3.12-pre
- GitHub Check: linux-cp3.12-install-from-sdist
- GitHub Check: linux-cp3.12-optimize and no pooch
- GitHub Check: linux-cp3.12-optional-deps
- GitHub Check: linux-cp3.11-default
- GitHub Check: linux-cp3.14-default
- GitHub Check: linux-cp3.11-mini-req-eager-import
- GitHub Check: linux-cp3.11-mini-req-optional-deps
- GitHub Check: linux-cp313t-default
- GitHub Check: macos-15-intel-cp3.13
- GitHub Check: windows-cp3.11-default
- GitHub Check: macos-latest-cp3.13
- GitHub Check: macos-latest-cp3.11
- GitHub Check: windows-cp3.11-optional-deps
- GitHub Check: windows-cp3.12-pre
- GitHub Check: windows-cp3.13-default
- GitHub Check: macos-docs
- GitHub Check: pyodide0.29.0-cp3.13-emscr4.0.9
🔇 Additional comments (6)
src/skimage/measure/pnpoly.py (1)
24-24: Docstring clarification looks good.The updated shape wording and explicit label meanings are clearer and align with standard docstring conventions.
Also applies to: 32-37
src/skimage/measure/_colocalization.py (3)
20-23: Docstring shape wording looks good.Clearer and consistent parameter shape description.
105-105: Docstring shape wording looks good.Matches the standardized format used elsewhere.
188-197: Docstring cleanup is clear and consistent.Shape wording and punctuation improvements read well.
src/skimage/filters/_median.py (2)
18-23: Footprint behavior clarification looks good.
Clear distinction betweenrankandndimagefootprints.
26-35: Docstring updates and example style are consistent.
Nice alignment with the standardized “ndarray of shape …” style and the module alias example usage.Also applies to: 46-48, 63-65
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/typing.yml:
- Around line 50-51: The docstub invocation uses an invalid workers value
"--workers -1" which can cause failures; update the docstub run command (the
line invoking "docstub run --verbose --group-errors --allow-errors=947 ...
src/skimage/") to either remove the "--workers -1" flag entirely to use the
default or replace it with a positive integer (for example "--workers 4") so the
flag passes a non-negative integer accepted by docstub 0.6.0.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/typing.ymlsrc/skimage/filters/_median.pysrc/skimage/measure/_colocalization.pysrc/skimage/measure/pnpoly.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/skimage/measure/_colocalization.py
- src/skimage/filters/_median.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: windows-cp3.12-pre
- GitHub Check: windows-cp3.13-default
- GitHub Check: windows-cp3.11-default
- GitHub Check: windows-cp3.11-optional-deps
- GitHub Check: linux-cp3.12-install-from-sdist
- GitHub Check: linux-cp3.11-default
- GitHub Check: linux-cp3.12-optional-deps
- GitHub Check: linux-cp3.11-mini-req-eager-import
- GitHub Check: macos-latest-cp3.13
- GitHub Check: linux-cp3.12-optimize and no pooch
- GitHub Check: linux-cp3.13-default
- GitHub Check: macos-15-intel-cp3.13
- GitHub Check: linux-cp3.12-pre
- GitHub Check: macos-latest-cp3.11
- GitHub Check: linux-cp3.11-mini-req-optional-deps
- GitHub Check: linux-cp3.14-default
- GitHub Check: linux-cp313t-default
- GitHub Check: pyodide0.29.0-cp3.13-emscr4.0.9
- GitHub Check: macos-docs
🔇 Additional comments (1)
src/skimage/measure/pnpoly.py (1)
24-37: Docstring updates look consistent and clearer.
The shape/label wording is now standardized and unambiguous.Also applies to: 51-64
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
lagru
left a comment
There was a problem hiding this comment.
I think staying with the status quo (dtype (uint8 or uint16)) is the better way to go for now.
I will push my requested changes right now but will keep this PR open for now to give you a chance to disagree. 😉
| Parameters | ||
| ---------- | ||
| image : ndarray of shape (M, N) and dtype (uint8 or uint16) | ||
| image : ndarray of shape (M, N) and dtype int |
There was a problem hiding this comment.
I'm sorry but I don't think I agree with this change (922710f).
Yes, you can technically pass dtype=np.int64 or similar as an image. But this will incur precision loss and a warning, urging you to convert to uint16 before.
This seems unexpected in context of dtype int. The reason we are doing this is so that type checkers can be useful and can catch issue like this beforehand. They won't if we use int here.
Instead I think we should treat this as an undocumented backwards compatible fallback.
There was a problem hiding this comment.
Ok, I see what you mean. I was focusing too much on the syntax debate about brackets. 🙏
I don't think I agree with the commit this reverts (922710f). Yes, you can technically pass dtype=np.int64 or similar as an `image`. But this will incur precision loss and a warning, urging you to convert to uint16 before. This seems unexpected in context of dtype int. The reason we are doing this is so that type checkers can be useful and can catch issue like this beforehand. They won't if we use int here. Instead, I think we should treat this as an undocumented backwards compatible fallback.
|
I won't disagree. I'm re-running the IO jobs that failed for time-out reasons and when they pass I'll merge. It will help me iterate faster. |
This PR is orthogonal to #8014. Edit: Here I have addressed the last item in the list of **errors** that docstub spits out. The last item in the list is a group of **warnings**. Co-authored-by: Lars Grüter <lagru@mailbox.org>
Description
5110c66 is the only addition wrt PR #8011.
Checklist
./doc/examplesfor new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.