Mask builders#42
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: AdamBajger <33228382+AdamBajger@users.noreply.github.com>
Co-authored-by: AdamBajger <33228382+AdamBajger@users.noreply.github.com>
Co-authored-by: AdamBajger <33228382+AdamBajger@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: AdamBajger <33228382+AdamBajger@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
|
/gemini |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
ratiopath/masks/mask_builders/aggregation.py (1)
11-12: Remove unusedCallableimport.The
Callableimport inside theTYPE_CHECKINGblock is not used in any type annotations within this file. Thecast()on line 87 uses a string literal"Callable[..., NDArray[np.uint16]]"which doesn't require the actual import.♻️ Proposed fix
if TYPE_CHECKING: - from collections.abc import Callable + passOr remove the entire
if TYPE_CHECKING:block if no other imports are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratiopath/masks/mask_builders/aggregation.py` around lines 11 - 12, Remove the unused Callable import inside the TYPE_CHECKING block in aggregation.py: since the string literal type used in cast("Callable[..., NDArray[np.uint16]]", ...) and no other type annotations reference Callable, delete the line importing Callable (or remove the entire if TYPE_CHECKING: block if it becomes empty) to eliminate the unused import.ratiopath/masks/mask_builders/mask_builder.py (2)
106-127: Minor: step comment numbering is inconsistent.The inline comments use duplicate step numbers ("1." at lines 96 and 106, "4." at lines 122 and 127). Consider renumbering for clarity: steps 1–5 in sequence.
📝 Suggested numbering fix
- # 1. Calculate how many tiles are required to fully cover the WSI (implicitly padding the edges) + # 2. Calculate how many tiles are required to fully cover the WSI (implicitly padding the edges) ... - # 4. Calculate the required MaskBuilder properties + # 4. Calculate the required MaskBuilder properties (unchanged) ... - # 4. Initialize Storage + # 5. Initialize Storage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratiopath/masks/mask_builders/mask_builder.py` around lines 106 - 127, The inline step numbering in mask_builder.py is inconsistent; update the comment numbers around the block that computes num_tiles, self.span, gcd, self.mask_extents, self.upscale_factor, and self.mask_stride so they form a single sequential sequence (e.g., 1–5) instead of repeating "1." and "4."; keep the existing explanatory text and positioning but renumber the comments for clarity (refer to symbols num_tiles, self.span, gcd, self.mask_extents, self.upscale_factor, self.mask_stride to locate the block).
195-211: Consider addingReturns:to docstring.The
resize_to_sourcemethod documentsArgs:but notReturns:. Per coding guidelines, Google-style docstrings should includeReturns:when applicable.📝 Proposed docstring addition
Args: image: Array of shape (C, H_mask, W_mask) to be resized to (H_source, W_source, C). + + Returns: + A pyvips.Image resized and cropped to match the original source extents. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratiopath/masks/mask_builders/mask_builder.py` around lines 195 - 211, The docstring for resize_to_source is missing a Returns section; update the resize_to_source(docstring) to include a Returns: paragraph stating it returns a pyvips.Image (vips_image) containing the resized mask at source resolution—i.e., a pyvips.Image with dimensions corresponding to (H_source, W_source, C) or described as "pyvips.Image resized/cropped to self.source_extents"—so callers and linters know the function's return type and semantics.ratiopath/masks/tissue_mask.py (1)
14-14: Parameterfiltershadows the built-infilterfunction.Using
filteras a parameter name shadows Python's built-in. Consider renaming tovips_filterorimage_filterfor clarity.Suggested rename
def tissue_mask( - slide: pyvips.Image, mpp: Res, filter: VipsFilter | None = None + slide: pyvips.Image, mpp: Res, vips_filter: VipsFilter | None = None ) -> pyvips.Image: ... - if filter is None: - filter = VipsCompose( + if vips_filter is None: + vips_filter = VipsCompose( [ VipsGrayScaleFilter(), VipsOtsu(), VipsClosing(), VipsOpening(), ] ) - return filter(slide, mpp)[0] + return vips_filter(slide, mpp)[0]Also applies to: 40-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratiopath/masks/tissue_mask.py` at line 14, The parameter name "filter" in the function signature and usages shadows Python's built-in; rename it (e.g., to vips_filter) across the function in tissue_mask.py: update the parameter name in the signature (slide: pyvips.Image, mpp: Res, vips_filter: VipsFilter | None = None), update all internal references to that parameter inside the function body, adjust any other functions or overloads mentioned around the 40-48 range that also use "filter", and update any callers to pass the renamed argument name where keyword usage is present; keep the original type hint VipsFilter | None and behavior unchanged.ratiopath/masks/write_big_tiff.py (1)
20-43: Consider adding aRaises:section to the docstring.The function can raise
ZeroDivisionErrorifmpp_xormpp_yis zero, and potentiallypyvips.Errorif the save operation fails. Per coding guidelines for Google-style docstrings, documenting possible exceptions improves API clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ratiopath/masks/write_big_tiff.py` around lines 20 - 43, Update the docstring for the save_big_tiff function in write_big_tiff.py to include a "Raises:" section that documents possible exceptions: mention ZeroDivisionError when mpp_x or mpp_y is zero and pyvips.Error (or pyvips.Error-like exceptions) when the pyvips.Image.tiffsave call fails; keep the wording concise and follow the existing Google-style docstring format so callers know which exceptions to expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ratiopath/masks/mask_builders/mask_builder.py`:
- Around line 185-187: The cached array returned by _prepare_clipping is being
mutated in-place when you do shift *= self.upscale_factor; instead, copy the
cached array before mutating so subsequent calls aren't corrupted — e.g.,
replace use of the returned clip_start array (variable shift) with a shallow
copy (or explicitly create a new array) and then scale and apply that copy to
coords; update the code around variables shift, coords and the call to
_prepare_clipping to ensure you never mutate the cached array in-place.
In `@ratiopath/masks/mask_builders/storage.py`:
- Line 45: The keyword-argument typo `kawargs` prevents extra kwargs from being
forwarded to open_memmap; update the MemMap call in storage.py to use the
correct `kwargs` identifier (replace `**kawargs` with `**kwargs`) so
MaskBuilder-provided keyword arguments are passed through to open_memmap/ MemMap
initialization.
---
Nitpick comments:
In `@ratiopath/masks/mask_builders/aggregation.py`:
- Around line 11-12: Remove the unused Callable import inside the TYPE_CHECKING
block in aggregation.py: since the string literal type used in
cast("Callable[..., NDArray[np.uint16]]", ...) and no other type annotations
reference Callable, delete the line importing Callable (or remove the entire if
TYPE_CHECKING: block if it becomes empty) to eliminate the unused import.
In `@ratiopath/masks/mask_builders/mask_builder.py`:
- Around line 106-127: The inline step numbering in mask_builder.py is
inconsistent; update the comment numbers around the block that computes
num_tiles, self.span, gcd, self.mask_extents, self.upscale_factor, and
self.mask_stride so they form a single sequential sequence (e.g., 1–5) instead
of repeating "1." and "4."; keep the existing explanatory text and positioning
but renumber the comments for clarity (refer to symbols num_tiles, self.span,
gcd, self.mask_extents, self.upscale_factor, self.mask_stride to locate the
block).
- Around line 195-211: The docstring for resize_to_source is missing a Returns
section; update the resize_to_source(docstring) to include a Returns: paragraph
stating it returns a pyvips.Image (vips_image) containing the resized mask at
source resolution—i.e., a pyvips.Image with dimensions corresponding to
(H_source, W_source, C) or described as "pyvips.Image resized/cropped to
self.source_extents"—so callers and linters know the function's return type and
semantics.
In `@ratiopath/masks/tissue_mask.py`:
- Line 14: The parameter name "filter" in the function signature and usages
shadows Python's built-in; rename it (e.g., to vips_filter) across the function
in tissue_mask.py: update the parameter name in the signature (slide:
pyvips.Image, mpp: Res, vips_filter: VipsFilter | None = None), update all
internal references to that parameter inside the function body, adjust any other
functions or overloads mentioned around the 40-48 range that also use "filter",
and update any callers to pass the renamed argument name where keyword usage is
present; keep the original type hint VipsFilter | None and behavior unchanged.
In `@ratiopath/masks/write_big_tiff.py`:
- Around line 20-43: Update the docstring for the save_big_tiff function in
write_big_tiff.py to include a "Raises:" section that documents possible
exceptions: mention ZeroDivisionError when mpp_x or mpp_y is zero and
pyvips.Error (or pyvips.Error-like exceptions) when the pyvips.Image.tiffsave
call fails; keep the wording concise and follow the existing Google-style
docstring format so callers know which exceptions to expect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43cc34c2-0ba4-4e18-bcc5-8d04ece7edfc
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.ruff.tomldocs/reference/masks/mask_builders.mdmkdocs.ymlpyproject.tomlratiopath/masks/__init__.pyratiopath/masks/mask_builders/__init__.pyratiopath/masks/mask_builders/aggregation.pyratiopath/masks/mask_builders/mask_builder.pyratiopath/masks/mask_builders/storage.pyratiopath/masks/tissue_mask.pyratiopath/masks/write_big_tiff.pyratiopath/parsers/darwin7_json_parser.pytests/test_mask_builders.py
✅ Files skipped from review due to trivial changes (6)
- .ruff.toml
- mkdocs.yml
- ratiopath/parsers/darwin7_json_parser.py
- pyproject.toml
- ratiopath/masks/init.py
- tests/test_mask_builders.py
Co-authored-by: Adam Kukučka <adam.kukucka4@gmail.com>
Remove ICC profile data from the image before saving.
This pull request introduces a new, modular mask-building subsystem for assembling feature masks from neural network tile predictions, along with comprehensive documentation and API exposure. It also updates the project version to 1.4.0 and makes minor configuration improvements. The main themes are the addition of the mask builders feature, documentation enhancements, and project configuration updates.
Mask Builders Subsystem:
mask_buildersmodule for assembling large feature masks from tiled neural network outputs, supporting automatic coordinate scaling, edge clipping, memory management (RAM or disk), and pluggable aggregation strategies (MeanAggregator,MaxAggregator). This includes the coreMaskBuilderclass, aggregation strategies, and storage backends. [1] [2] [3]mask_buildersAPI in the package__init__.pyfor external use.Documentation:
mkdocs.ymlto include the new mask builders and parser documentation. [1] [2]Project Configuration:
.ruff.tomlfrom 3.11 to 3.12 for linting and formatting.Other Exports:
write_big_tifffrom theratiopath.maskspackage for broader accessibility.