refactor: residual coverage, uv, and migration #2
Conversation
…er python target in lock file
See merge request rationai/digital-pathology/quality-control/quality-control!7
📝 WalkthroughWalkthroughAdds GitHub Actions workflows and lint/config files, removes GitLab CI, updates project metadata and docs, and refactors QC modules to use StandardConversions, change several return shapes to per-pixel masks plus number_of_examined_pixels/number_of_flagged_pixels, and update typing and utils signatures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request encompasses a significant overhaul of the project's development and deployment infrastructure, alongside critical updates to its core image quality control functionalities. The migration from GitLab to GitHub, coupled with the adoption of Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request updates CI/CD and linting configurations, including upgrading commitizen and ruff, and adding specific linting exclusions for markdown and Python type checking. Documentation for installation and usage examples has been significantly revised, introducing uv for dependency management and clarifying code snippets for residual artifact, blur, and fold detection. Core QC functions (blur_score_laplacian, blur_score_piqe, blur_score_roberts, folding, residual_artifacts_and_coverage) have been refactored to standardize return values, improve type hinting, and update morphological operations. Review comments highlight a potential UnboundLocalError in the folding function, semantic and usage issues with the tissue_img parameter in blur utility functions, typos and inconsistencies in documentation code examples for fold detection, and a type mismatch in the blur_score_piqe function's return value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/getting-started/quality-control.md (2)
193-233:⚠️ Potential issue | 🟠 MajorThe folding walkthrough is out of sync with the shown API.
As written, this section is not runnable:
foldingis never imported,pixel_sizeis undefined,local_area_image/img_area/local_area_imgare mixed, and the prose below still explainslevel_downsample/nuclues_diameter_at_base_leveleven though the example callsfolding(..., mpp=1.76, ...).💡 Proposed fix
+from rationai.qc.folding import folding ... -img = np.asarray(Image.open("fold.png").convert("RGB")) # Investigated image -local_area_image =img_area = np.asarray(Image.open("fold_area.png").convert("RGB")) # Local area of investigated image +img = np.asarray(Image.open("fold.png").convert("RGB")) # Investigated image +local_area_img = np.asarray(Image.open("fold_area.png").convert("RGB")) # Local area of investigated image +mpp = 1.76 ... img_mask = tissue_mask( - pyvips.Image.new_from_array(img), mpp=pixel_size).numpy() > 0 + pyvips.Image.new_from_array(img), mpp=mpp).numpy() > 0 img_area_mask = tissue_mask( - pyvips.Image.new_from_array(local_area_img), mpp=pixel_size).numpy() > 0 + pyvips.Image.new_from_array(local_area_img), mpp=mpp).numpy() > 0 ... -The `level_downsample argument` is the downsample rate between the highest resolution level and the level from which the image was taken. -`nuclues_diameter_at_base_level` specifies the nucleus diameter at the highest resolution level (it can be easily measured when browsing the WSI). +The `mpp` argument is the pixel size of the analyzed image in micrometers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/quality-control.md` around lines 193 - 233, The walkthrough is broken: update the sample to import the actual folding API (e.g., add from rationai.artifacts import folding), define pixel_size (or consistently use mpp) and pass it into tissue_mask, and consistently name the local area variables (use local_area_image everywhere, not img_area/local_area_img); also align the prose with the call by removing or updating the outdated mentions of level_downsample/nuclues_diameter_at_base_level (or describe mpp and nucleus_diameter_at_base_level if those args are used) so the example and documentation refer to the same parameters and names.
83-117:⚠️ Potential issue | 🟡 MinorImport
blur_score_piqebefore calling it in the sample.This example invokes
blur_score_piqe(...)three times, but the import block never brings that symbol into scope, so the snippet raisesNameError.💡 Proposed fix
import pyvips from rationai.masks import tissue_mask +from rationai.qc.blur import blur_score_piqe🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/quality-control.md` around lines 83 - 117, Add the missing import for blur_score_piqe (e.g., add "from rationai.blur import blur_score_piqe" to the top import block) so blur_score_piqe is in scope before it's called; also avoid shadowing the tissue_mask function by renaming the mask variable (e.g., mask = tissue_mask(...).numpy()) and use that renamed variable when calling blur_score_piqe(img_c, pixel_size, mask).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/mkdocs-build.yml:
- Line 11: Replace the branch reference in the reusable workflow call so it is
pinned to an exact commit SHA instead of using `@main`; locate the `uses:
RationAI/.github/.github/workflows/mkdocs-build.yml@main` entry and update the
suffix to the full commit SHA from the RationAI/.github repository (obtain the
SHA for the desired commit and substitute it, e.g., `@<full-commit-sha>`),
commit the change so the workflow now references an immutable revision.
In @.pre-commit-config.yaml:
- Around line 19-22: Pre-commit currently calls the Ruff hooks (id: ruff and id:
ruff-format) without enabling Ruff's --force-exclude, so pre-commit-staged files
will bypass extend-exclude entries; update the hooks in .pre-commit-config.yaml
so the ruff hook (id: ruff) includes --force-exclude alongside existing args
(e.g., add --force-exclude to the args array where --fix is set) and ensure the
ruff-format hook (id: ruff-format) also runs with --force-exclude so configured
exclusions in .ruff.toml are respected during pre-commit runs.
In `@docs/getting-started/installation.md`:
- Around line 23-29: Update the "uv" tab example in
docs/getting-started/installation.md to use the pip workflow or clarify its
preconditions: either change the shown command from `uv add
git+https://github.com/RationAI/quality-control.git` to the pip-equivalent `uv
pip install git+https://github.com/RationAI/quality-control.git`, or add a brief
note that `uv add` updates pyproject.toml and requires an existing uv project
structure. Ensure you modify the code block inside the "uv" tab (the
triple-backtick bash block) and/or add one short sentence explaining the
difference so users know when to use `uv add` vs `uv pip install`.
In `@docs/getting-started/quality-control.md`:
- Around line 57-63: The example prints use nested double quotes which make the
f-strings invalid; update the two print statements to use either single quotes
for the dict keys or escape the inner quotes so that the f-strings are valid
(e.g., change artifacts["number_of_flagged_pixels"] and
artifacts["number_of_examined_pixels"] to artifacts['number_of_flagged_pixels']
and artifacts['number_of_examined_pixels'] in the print calls). Ensure you only
modify the quote characters in the two print(...) lines and leave the rest (mask
creation and save) unchanged.
In `@pyproject.toml`:
- Line 20: The pyproject dependency entries for rationai-masks and
rationai-staining are unpinned and still reference the GitLab HEAD; update the
dependency strings for both rationai-masks and rationai-staining to pin them to
explicit git revisions using the uv-supported syntax (e.g., append @<rev-or-tag>
after the .git URL or use `@tag`=<tag>) so installs are deterministic and match
the docs; modify the entries that reference "rationai-masks" and
"rationai-staining" accordingly to include a specific commit hash or tag.
In `@rationai/qc/folding/folding.py`:
- Around line 147-164: The non-H&E return path references folding_test but it is
only created inside the hematoxylin_eosin_stained branch; initialize or assign
folding_test before the if so both paths return a defined value—e.g., compute
folding_test using reconstruction(folding_test_markers, thresholded_eosin) or
set folding_test = np.zeros_like(thresholded_eosin, dtype=...) when
hematoxylin_eosin_stained is False so np.count_nonzero(folding_test) and the
returned "folding_per_pixel" are valid (refer to variables folding_test,
reconstruction, thresholded_eosin, hematoxylin_eosin_stained).
- Around line 29-31: The local-threshold branch can call threshold_yen on an
empty array when local_mask or mask have no tissue; update the conditional
around the MaskedArray usage (the branch that uses local_tiles/local_mask and
the fallback using img/mask) to check that MaskedArray(...).compressed() yields
a non-empty array before calling threshold_yen, and if empty fall back to the
other available non-empty compressed array (or a default behavior) so
threshold_yen is never invoked on a zero-length array; reference the existing
symbols local_tiles, local_mask, MaskedArray, threshold_yen, img, and mask to
locate and modify the logic.
In `@rationai/qc/typing.py`:
- Around line 58-62: The docstring for the TypedDict class ResidualArtifacts is
stale — it still says the type exposes a "coverage mask" while the actual field
name artifacts_per_pixel (and type BinaryMask) indicates it's a mask of detected
residual artifacts; update the class docstring to accurately describe that
ResidualArtifacts contains a binary mask of detected residual artifacts and
counts for examined/flagged pixels, ensuring the wording aligns with the
artifacts_per_pixel field and any other fields in ResidualArtifacts.
---
Outside diff comments:
In `@docs/getting-started/quality-control.md`:
- Around line 193-233: The walkthrough is broken: update the sample to import
the actual folding API (e.g., add from rationai.artifacts import folding),
define pixel_size (or consistently use mpp) and pass it into tissue_mask, and
consistently name the local area variables (use local_area_image everywhere, not
img_area/local_area_img); also align the prose with the call by removing or
updating the outdated mentions of
level_downsample/nuclues_diameter_at_base_level (or describe mpp and
nucleus_diameter_at_base_level if those args are used) so the example and
documentation refer to the same parameters and names.
- Around line 83-117: Add the missing import for blur_score_piqe (e.g., add
"from rationai.blur import blur_score_piqe" to the top import block) so
blur_score_piqe is in scope before it's called; also avoid shadowing the
tissue_mask function by renaming the mask variable (e.g., mask =
tissue_mask(...).numpy()) and use that renamed variable when calling
blur_score_piqe(img_c, pixel_size, mask).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97b86365-670a-4c89-bfd9-d1b7a5ae1016
⛔ Files ignored due to path filters (2)
pdm.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/mkdocs-build.yml.github/workflows/python-lint.yml.gitlab-ci.yml.markdownlint.yaml.mypy.ini.pre-commit-config.yaml.ruff.tomlREADME.mddocs/getting-started/installation.mddocs/getting-started/quality-control.mddocs/index.mdpyproject.tomlrationai/qc/blur/blur_score_laplacian.pyrationai/qc/blur/blur_score_piqe.pyrationai/qc/blur/blur_score_roberts.pyrationai/qc/blur/utils.pyrationai/qc/folding/folding.pyrationai/qc/residual_artifacts/residual_artifacts_and_coverage.pyrationai/qc/staining/color_difference.pyrationai/qc/staining/dominant_stains.pyrationai/qc/staining/staining_difference.pyrationai/qc/typing.py
💤 Files with no reviewable changes (1)
- .gitlab-ci.yml
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 (1)
rationai/qc/folding/folding.py (1)
144-153:⚠️ Potential issue | 🔴 CriticalUse a 2D boolean zero mask for
thresholded_valuereset.Line 149 currently resets
thresholded_valuewithnp.zeros(tile.shape), which creates a 3D float array. This causes a runtimeTypeErrorat line 152 when performing bitwise AND operations with 2D boolean masks:TypeError: ufunc 'bitwise_and' not supported for the input types, and the inputs could not be safely coerced to any supported typesThe fix should maintain the 2D boolean shape consistent with
thresholded_saturationandthresholded_eosin:Suggested patch
- thresholded_value = np.zeros(tile.shape) + thresholded_value = np.zeros_like(thresholded_value, dtype=bool)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rationai/qc/folding/folding.py` around lines 144 - 153, The reset of thresholded_value currently uses np.zeros(tile.shape) producing a 3D float array and breaking bitwise ops with 2D boolean masks; change the reset to create a 2D boolean mask instead (e.g. use np.zeros(thresholded_value.shape, dtype=bool) or np.zeros(tile.shape[:2], dtype=bool)) so thresholded_value remains a 2D boolean array compatible with the subsequent opening(...) and bitwise AND operations (see thresholded_value, thresholded_saturation, thresholded_eosin and the opening(...) call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started/quality-control.md`:
- Line 15: Replace the misspellings in the user-facing docs: change "neccessary"
to "necessary" (found in the sentence "The following examples should provide you
with the neccessary information..." near Line 15) and change "computated" to
"computed" (found near Line 121); update both occurrences in
docs/getting-started/quality-control.md so the documentation reads correctly.
---
Outside diff comments:
In `@rationai/qc/folding/folding.py`:
- Around line 144-153: The reset of thresholded_value currently uses
np.zeros(tile.shape) producing a 3D float array and breaking bitwise ops with 2D
boolean masks; change the reset to create a 2D boolean mask instead (e.g. use
np.zeros(thresholded_value.shape, dtype=bool) or np.zeros(tile.shape[:2],
dtype=bool)) so thresholded_value remains a 2D boolean array compatible with the
subsequent opening(...) and bitwise AND operations (see thresholded_value,
thresholded_saturation, thresholded_eosin and the opening(...) call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 799b67ee-252e-41ab-be2b-5f931fd50319
📒 Files selected for processing (5)
.pre-commit-config.yamldocs/getting-started/installation.mddocs/getting-started/quality-control.mdrationai/qc/folding/folding.pyrationai/qc/typing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/getting-started/quality-control.md (1)
106-113: Avoid shadowingtissue_maskin the example snippet.At Line 106 and Line 110-113,
tissue_maskis used as both function and variable name. This is a common Python pitfall for readers who extend the snippet.💡 Suggested patch
-tissue_mask = tissue_mask( +tissue_mask_arr = tissue_mask( pyvips.Image.new_from_array(img_c), mpp=pixel_size ).numpy() -tissue_mask = (tissue_mask > 0).astype(int) # binarize mask +tissue_mask_arr = (tissue_mask_arr > 0).astype(int) # binarize mask # blur score with tissue mask -blur_score_c = blur_score_piqe(img_c, pixel_size, tissue_mask) +blur_score_c = blur_score_piqe(img_c, pixel_size, tissue_mask_arr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/quality-control.md` around lines 106 - 113, The example shadows the tissue_mask function by reassigning it to a variable; change the variable name (e.g., tissue_mask_arr or tissue_mask_img) so you do not override the tissue_mask function, update the binarize line to assign to that new name, and pass the new variable into blur_score_piqe(img_c, pixel_size, <new_variable>) to avoid function/variable name collision while keeping tissue_mask() as the callable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started/quality-control.md`:
- Around line 3-4: Replace the three phrasing issues in the document: change
"good overview" to "a good overview" where the intro sentence uses that phrase,
change "Function gives" to "The function gives" where the sentence begins with
"Function gives…", and ensure the sentence ending with "of the investigated
image" has a trailing period (replace any variant without a period). Locate
these exact phrases in the text and perform the replacements to improve clarity.
---
Nitpick comments:
In `@docs/getting-started/quality-control.md`:
- Around line 106-113: The example shadows the tissue_mask function by
reassigning it to a variable; change the variable name (e.g., tissue_mask_arr or
tissue_mask_img) so you do not override the tissue_mask function, update the
binarize line to assign to that new name, and pass the new variable into
blur_score_piqe(img_c, pixel_size, <new_variable>) to avoid function/variable
name collision while keeping tissue_mask() as the callable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76d2ff9c-ad2c-4702-b8bd-a572a4deb091
📒 Files selected for processing (1)
docs/getting-started/quality-control.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started/quality-control.md`:
- Line 57: The mask construction uses Image.fromarray with
artifacts["artifacts_per_pixel"].astype(np.uint8) before scaling, which can
truncate values; change the expression to perform the scaling first and then
cast to uint8 (i.e., multiply the float array by 255, then call
.astype(np.uint8)) so that the mask assignment (the line building mask via
Image.fromarray) uses the scaled-then-cast array; apply the same change for the
other occurrence around line 239 to match the blur examples' safer pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 366120b4-2c7e-4d2e-9ff4-7ba1bc45e170
📒 Files selected for processing (1)
docs/getting-started/quality-control.md
| ```python linenums="19" | ||
| mask = Image.fromarray(255 * artifacts["coverage_mask"].astype(np.uint8)) | ||
| ```python linenums="18" | ||
| mask = Image.fromarray(255 * artifacts["artifacts_per_pixel"].astype(np.uint8)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n docs/getting-started/quality-control.md | sed -n '50,65p;235,245p'Repository: RationAI/quality-control
Length of output: 1351
🏁 Script executed:
# Also check if the other lines mentioned (132, 135, 138, 144, 147, 150) exist and what patterns they use
cat -n docs/getting-started/quality-control.md | sed -n '130,155p'Repository: RationAI/quality-control
Length of output: 1042
Cast after scaling when building masks for Image.fromarray.
Lines 57 and 239 apply astype(np.uint8) before the multiplication, which is inconsistent with the safer pattern used in the blur examples (lines 132, 135, 138, 144, 147, 150). Move the cast after the scaling operation to ensure proper type handling.
Suggested patch
-mask = Image.fromarray(255 * artifacts["artifacts_per_pixel"].astype(np.uint8))
+mask = Image.fromarray((255 * artifacts["artifacts_per_pixel"]).astype(np.uint8))-mask = Image.fromarray(255 * artifacts["folding_per_pixel"].astype(np.uint8))
+mask = Image.fromarray((255 * artifacts["folding_per_pixel"]).astype(np.uint8))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/getting-started/quality-control.md` at line 57, The mask construction
uses Image.fromarray with artifacts["artifacts_per_pixel"].astype(np.uint8)
before scaling, which can truncate values; change the expression to perform the
scaling first and then cast to uint8 (i.e., multiply the float array by 255,
then call .astype(np.uint8)) so that the mask assignment (the line building mask
via Image.fromarray) uses the scaled-then-cast array; apply the same change for
the other occurrence around line 239 to match the blur examples' safer pattern.
2 participants
This pull request includes the following changes:
pdmwith theuvdependency manager.Unfortunately, I couldn't think of a clean way to merge only the changes required for the repo migration (the code in the master branch would not be runnable, and linting checks would not pass due to typing and linting errors, and the breaking update to the Staining library in the meantime).
Any suggestions for improvement are appreciated. 😄
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Chores