Repository navigation
refactor: typing and api - #5
TomasBalak wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Project tooling and development setup .github/workflows/*, .gitlab-ci.yml, pyproject.toml, .pre-commit-config.yaml, .ruff.toml, .mypy.ini, .markdownlint.yaml, README.md, docs/getting-started/installation.md, docs/index.md |
Shared GitHub workflows replace the GitLab CI configuration. Development dependencies use dependency groups. Ruff, mypy, Markdownlint, pre-commit, package metadata, and installation instructions are updated. |
Quality-control contracts and color conversions rationai/qc/typing.py, rationai/qc/staining/* |
Image and result types now expose floating-point image data, per-pixel masks, and examined or flagged pixel counts. Staining code uses the current conversion direction, matrix, and standard conversions. |
Blur processing and pixel counts rationai/qc/blur/*, docs/getting-started/quality-control.md |
Blur implementations use updated morphology and conversion APIs, apply foreground masks, and return examined or flagged pixel counts. Blur documentation matches the updated outputs. |
Residual-artifact result changes rationai/qc/residual_artifacts/*, docs/getting-started/quality-control.md |
Residual-artifact analysis returns artifacts_per_pixel, number_of_examined_pixels, and number_of_flagged_pixels instead of coverage data. |
Fold detection and neighborhood thresholds rationai/qc/folding/folding.py, docs/getting-started/quality-control.md |
Fold detection renames neighborhood parameters, handles empty masked data, uses updated morphology and conversions, and returns folding_per_pixel with pixel counts. |
Estimated code review effort: 4 (Complex) | ~45 minutes
🚥 Pre-merge checks | ✅ 5
-
✅ Passed checks (5 passed)
-
Check name Status Explanation Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request. Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled. Title check ✅ Passed The title accurately summarizes the main changes to typing and API usage across the quality-control modules.
-
Finishing Touches
-
Fix failing CI checks
-
- Fix failing CI checks
Generate docstrings
-
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
refactor/typing-and-api
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
-
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
-
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
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.
CodeRabbit Verify
- Run CodeRabbit Verify to test the changes in this pull request.
![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
This pull request migrates the project's dependency management from PDM to uv, updates pre-commit and linter configurations, and standardizes the return structures of several quality control functions to consistently report the number of examined and flagged pixels. It also replaces deprecated scikit-image morphological operations with their modern equivalents and updates the documentation accordingly. The review identified a critical logic bug in the folding safety check where a condition is mathematically impossible to satisfy and creates a 3D array instead of a 2D array, a shape mismatch in the initialization of local_eosin_channel, and a potential shape mismatch in blur_score_piqe.py when image dimensions are not divisible by 16.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Add FloatingPointImage type alias and update return type annotations - Rename fold detection key from 'folding' to 'folding_per_pixel' - Rename residual artifacts key from 'coverage_mask'/'coverage' to 'artifacts_per_pixel'/'number_of_examined_pixels'/'number_of_flagged_pixels' - Add number_of_examined_pixels and number_of_flagged_pixels to all blur score and fold artifact return dictionaries - Replace deprecated binary_dilation/binary_erosion with dilation/erosion - Replace deprecated ColorConversion with StandardConversions API - Update color_difference.py for new staining library API - Simplify blur score logic and fix shadowing issues
689becd to
7961a57
Compare
| thresholded_value = inverted_value_channel > value_threshold | ||
| thresholded_eosin = eosin_channel > eosin_threshold | ||
|
|
||
| if ( |
There was a problem hiding this comment.
See gemini-code-assist[bot] comment on lines R151 to R154: https://github.com/RationAI/quality-control/pull/5/changes#r3383017764
If this does work as intended, please state the intended behavior/purpose explicitly in a comment in the code.
There was a problem hiding this comment.
I am not that familiar with this folding code, so I've forwarded the issue to Erik (I couldn't find his username here), I believe he will be able to provide a better answer.
There was a problem hiding this comment.
I'm still missing the intended behavior/purpose of this check. What state is the condition (with several or-ed np.sum(...) * 2 > np.prod(tile.shape[:2]) terms) describing?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/mkdocs-build.yml:
- Line 11: Pin the reusable workflow references in
.github/workflows/mkdocs-build.yml:11 and .github/workflows/python-lint.yml:11
to approved full commit SHAs instead of the mutable `@main` ref, preserving the
existing RationAI/.github workflow targets.
In @.ruff.toml:
- Around line 5-6: Add the top-level Ruff setting force-exclude = true alongside
extend-exclude in .ruff.toml so pre-commit hooks honor the exclusion for
explicitly passed filenames.
In `@docs/getting-started/installation.md`:
- Line 1: Update the numbered H3 headings in the installation documentation,
including the pyvips section and the section at the corresponding later
occurrence, by moving each numeric prefix after the H3 marker (for example, “###
1. …”). Preserve the existing section titles and numbering.
In `@docs/getting-started/quality-control.md`:
- Around line 226-231: Update the documented folding() invocation to use the
current keyword arguments neighborhood_tiles and neighborhood_mask instead of
local_tiles and local_mask, while preserving the existing values
local_area_image and img_area_mask.
🪄 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 Plus
Run ID: 0134f611-dd5f-4eb0-8313-0610953fb747
⛔ 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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| local_tiles: Optional n*n tiles in local neighbourhood of tile. Defaults to None. | ||
| local_mask: Optional n*n background mask of local_tiles. Defaults to None. | ||
| neighborhood_tiles: Optional n*n tiles in local neighbourhood of tile. Defaults to None. | ||
| neighborhood_mask: Optional n*n background mask of neighborhood_tiles. Defaults to None. |
There was a problem hiding this comment.
Why is neighborhood_mask a background mask here? On line 65, neighborhood_mask: Tissue mask of neighborhood_tiles. It should be kept consistent (the same name should have the same semantic), and also match the expectation that usually, when we are using a mask, it's a mask of the thing in question, not of its inverse/surrounding. (Here, on line 34, it gets inverted anyway: local_values = MaskedArray(neighborhood_tiles, ~neighborhood_mask).compressed().)
(Be careful about all places that call this function, make sure the "(non)invertedness" of the arguments matches what the function works with.)
| thresholded_value = inverted_value_channel > value_threshold | ||
| thresholded_eosin = eosin_channel > eosin_threshold | ||
|
|
||
| if ( |
There was a problem hiding this comment.
I'm still missing the intended behavior/purpose of this check. What state is the condition (with several or-ed np.sum(...) * 2 > np.prod(tile.shape[:2]) terms) describing?
Summary
This PR fixes typing issues, unifies return value naming across all QC modules, migrates to the refactored staining library API, and replaces deprecated scikit-image functions.
Note
This PR is a part of a two-stage PR based on #2. The original PR was split in order to be more "review-friendly." This PR should be merged after PR #4 is merged.
Changes
Typing System
FloatingPointImagetype alias (NDArray[np.float64])BlurScore,FoldArtifacts,ResidualArtifactsTypedDicts withnumber_of_examined_pixelsandnumber_of_flagged_pixelsfieldsNDArraytype hints across all modules for better type safetyAPI Consistency
coverage_mask→artifacts_per_pixel, replacecoverage(float ratio) with raw pixel countsnumber_of_examined_pixels/number_of_flagged_pixelsfolding→folding_per_pixelnumber_of_examined_pixels/number_of_flagged_pixelsto return dictionaries; rename internalblur_score_per_pixelvariable toblur_score_pooledto avoid shadowing return keyStaining Library API Migration
ColorConversion.RGB2HER→StandardConversions.RGB2HERConversionType→ConversionDirectionwith.conv_type→.direction.value[0][index]→.matrix[index]for color conversion matrix accessDeprecation Fixes
skimage.morphology.binary_dilation/binary_erosionwithdilation/erosion> 0casts, use in-placeoperators)
Bug Fixes
NoneSummary by CodeRabbit
New Features
Documentation
uv.Developer Experience