refactor: typing and api - #5
Conversation
- Replace GitLab CI with GitHub Actions workflows - Migrate from pdm to uv - Update pre-commit config, ruff, mypy, and markdownlint configs - Update documentation URLs from GitLab to GitHub - Reorganize pyproject.toml for uv compatibility
- Fix import ordering across all modules - Fix folding threshold calculation for empty masks - Apply ruff auto-fixes for consistency
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe project now uses shared GitHub workflows and updated development tooling. Quality-control APIs return per-pixel masks with examined and flagged pixel counts. Blur, residual-artifact, folding, staining, dependency, and documentation examples were updated for the new contracts. ChangesQuality-control modernization
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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.
| folding_test_markers = opening( | ||
| thresholded_eosin & thresholded_saturation & thresholded_value, | ||
| disk(cell_nucleus_size // (mpp)), | ||
| ) |
There was a problem hiding this comment.
There is a critical logic bug in the preceding if statement (lines 144-148) that directly impacts this line:\n\n1. Impossible Condition: np.sum(thresholded_value) * 2 > tile.size compares a 2D mask sum (max H * W) against tile.size (which is H * W * 3 for an RGB image). Thus, the condition is mathematically impossible to satisfy, and the safety check never triggers. It should compare against tile.shape[0] * tile.shape[1] or tile.size // 3.\n2. Shape Mismatch: If the condition were ever met, thresholded_value = np.zeros(tile.shape) would create a 3D array of shape (H, W, 3) instead of a 2D array of shape (H, W). This would cause a ValueError or unexpected behavior on line 152 during the bitwise AND operation thresholded_eosin & thresholded_saturation & thresholded_value.\n\nTo fix this, the preceding block should be updated to:\npython\n num_pixels = tile.shape[0] * tile.shape[1]\n if (\n np.sum(thresholded_value) * 2 > num_pixels\n or np.sum(thresholded_saturation) * 2 > num_pixels\n or (hematoxylin_eosin_stained and np.sum(thresholded_eosin) * 2 > num_pixels)\n ):\n thresholded_value = np.zeros(tile.shape[:2], dtype=bool)\n
| eosin_channel = np.ones_like(tissue_mask) | ||
| if local_tiles is not None: | ||
| local_eosin_channel = np.ones_like(local_tiles) | ||
| local_eosin_channel = np.ones_like(local_tiles, dtype=np.float64) |
There was a problem hiding this comment.
local_tiles is a 3D RGB image of shape (H_local, W_local, 3). Using np.ones_like(local_tiles) creates a 3D array of shape (H_local, W_local, 3). However, local_eosin_channel is expected to be a 2D single-channel array of shape (H_local, W_local) to match eosin_channel and other channels.\n\nWe should initialize it as a 2D array using the spatial dimensions of local_tiles.
| local_eosin_channel = np.ones_like(local_tiles, dtype=np.float64) | |
| local_eosin_channel = np.ones(local_tiles.shape[:2], dtype=np.float64) |
| # activity_mask is multiplied by the foreground mask to nullify background pixels | ||
| activity_mask = activity_mask * foreground_mask | ||
| # Invert and restrict the mask to foreground | ||
| activity_mask = foreground_mask * ~(activity_mask > 0) |
There was a problem hiding this comment.
If grayscale_img.shape != activity_mask.shape due to the height not being divisible by 16, the padding logic above (which only pads the width/columns) will not prevent a shape mismatch here, leading to a ValueError during multiplication.\n\nConsider updating the padding logic earlier in the function to dynamically pad both dimensions:\npython\n if grayscale_img.shape != activity_mask.shape:\n pad_h = grayscale_img.shape[0] - activity_mask.shape[0]\n pad_w = grayscale_img.shape[1] - activity_mask.shape[1]\n activity_mask = np.pad(activity_mask, ((0, pad_h), (0, pad_w)), mode="edge")\n
Both ColorConversion and convert_color are now imported from rationai.staining, consistent with the rest of the codebase.
- 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.
![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
|
|
||
| jobs: | ||
| run: | ||
| uses: RationAI/.github/.github/workflows/mkdocs-build.yml@main |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow lines =="
for f in .github/workflows/mkdocs-build.yml .github/workflows/python-lint.yml; do
if [ -f "$f" ]; then
echo "-- $f --"
nl -ba "$f" | sed -n '1,25p'
else
echo "MISSING $f"
fi
done
echo
echo "== all reusable workflow refs to RationAI/.github in workflows =="
rg -n "uses:\s*RationAI/\.github/\.github/workflows/.*@" .github/workflows || trueRepository: RationAI/quality-control
Length of output: 264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files =="
git ls-files '.github/workflows/*' || true
echo
echo "== workflow lines =="
for f in .github/workflows/mkdocs-build.yml .github/workflows/python-lint.yml; do
if [ -f "$f" ]; then
echo "-- $f --"
awk '{printf "%4d:\t%s\n", NR, $0}' "$f" | sed -n '1,40p'
else
echo "MISSING $f"
fi
done
echo
echo "== all reusable workflow refs to RationAI/.github in workflows =="
grep -RInE "uses:\s*RationAI/\.github/\.github/workflows/.*@" .github/workflows || trueRepository: RationAI/quality-control
Length of output: 1200
Pin the reusable workflows to full commit SHAs.
Both workflows call RationAI/.github with the mutable @main ref, so an upstream workflow change runs in this repository without a local review. Use an approved full commit SHA at .github/workflows/mkdocs-build.yml:11 and .github/workflows/python-lint.yml:11.
📍 Affects 2 files
.github/workflows/mkdocs-build.yml#L11-L11(this comment).github/workflows/python-lint.yml#L11-L11
🤖 Prompt for 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.
In @.github/workflows/mkdocs-build.yml at 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.
| extend-exclude = ["rationai/qc/blur/piqe.py"] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(\.ruff\.toml|\.pre-commit-config\.yaml)$' || true
echo
echo "== .ruff.toml =="
if [ -f .ruff.toml ]; then cat -n .ruff.toml; fi
echo
echo "== .pre-commit-config.yaml relevant hooks =="
if [ -f .pre-commit-config.yaml ]; then
rg -n "ruff-check|ruff-format|files:|force-exclude|extend-exclude" .pre-commit-config.yaml -C 2 || true
sed -n '1,70p' .pre-commit-config.yaml | cat -n
fi
echo
echo "== references to piqe.py =="
rg -n "piqe\.py|rationai/qc/blur/piqe|ruff" . -g '*.py' -g '*.toml' -g '*.yaml' -g '*.yml' || trueRepository: RationAI/quality-control
Length of output: 3464
🌐 Web query:
Ruff force-exclude explicit filenames documentation
💡 Result:
By default, Ruff analyzes all files passed directly to the command-line, even if they match patterns defined in your exclude or extend-exclude settings [1][2]. To force Ruff to respect your exclusion rules even when files are passed explicitly, you must enable the force-exclude option [1][2]. You can enable this setting in two ways: 1. Configuration File: Add force-exclude = true to your configuration file (e.g., pyproject.toml or ruff.toml) under the [tool.ruff] section [2]. [tool.ruff] force-exclude = true 2. Command-Line Interface: Pass the --force-exclude flag when running the ruff command [1][3]. This feature is particularly useful for tools like pre-commit, which often pass specific, modified files to Ruff regardless of its internal configuration [2][4]. Enabling force-exclude ensures these files remain subject to your project's defined exclusion patterns [2].
Citations:
- 1: https://docs.astral.sh/ruff/configuration/?featured_on=pythonbytes
- 2: https://docs.astral.sh/ruff/settings/
- 3: https://docs.astral.sh/ruff/configuration/
- 4: Files that are ignored in config are checked when passed explicitly on the command line astral-sh/ruff#1301
Make the exclusion apply to pre-commit hooks.
The Ruff pre-commit hooks pass changed filenames, and extend-exclude does not exclude explicit filenames unless force-exclude is enabled. Add force-exclude = true to the top-level Ruff settings, or pass --force-exclude to both ruff-check and ruff-format.
Proposed fix
extend-exclude = ["rationai/qc/blur/piqe.py"]
+force-exclude = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extend-exclude = ["rationai/qc/blur/piqe.py"] | |
| extend-exclude = ["rationai/qc/blur/piqe.py"] | |
| force-exclude = true |
🤖 Prompt for 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.
In @.ruff.toml around lines 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.
| @@ -1,4 +1,4 @@ | |||
| 1. ### Install **pyvips** for Efficient Image Processing | |||
| 1. ### Install **pyvips** for Efficient Image Processing | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: no numbered-list markers before Markdown headings.
! rg -n '^[0-9]+\. ### ' docs/getting-started/installation.mdRepository: RationAI/quality-control
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Markdown availability:"
command -v markdown || true
command -v pandoc || true
command -v node || true
echo
echo "Target lines:"
sed -n '1,30p' docs/getting-started/installation.md | cat -n
echo
echo "Parse probe with markdown-it (if available):"
python3 - <<'PY'
try:
import markdown_it
except Exception as e:
print(f"markdown_it unavailable: {type(e).__name__}: {e}")
raise SystemExit(0)
source = open("docs/getting-started/installation.md").read()
tokens = markdown_it.MarkdownIt().parse(source)
for token in tokens:
print(f"{token.type}\ttag={token.tag}\tmap={token.map}\theading= h{token.attrs[0]['level']} if token.type=='heading' else None\tcontent={token.content[:80]}")
PY
echo
echo "Parse probe with commonmark if installed:"
python3 - <<'PY'
try:
import commonmark
except Exception as e:
print(f"commonmark unavailable: {type(e).__name__}: {e}")
raise SystemExit(0)
source = open("docs/getting-started/installation.md").read()
reader = commonmark.BlockReader(source)
parser = commonmark.Parser()
doc = parser.parse(reader)
def dump(node, indent=0):
if node.type == "heading":
print("\t"*indent + f"heading level {node.level} text {next(node.blocks, type('', (), {'info': None})()).literal if hasattr(next(node.blocks, None), 'literal') else '(nested)'}")
elif node.type == "list":
print("\t"*indent + f"list tight={node.tight} delim={node.delim} start={node.start}")
sub = node.first_child
while sub:
dump(sub, indent+1)
sub = sub.next
dump(doc)
PYRepository: RationAI/quality-control
Length of output: 1593
🌐 Web query:
CommonMark numbered list marker before heading syntax
💡 Result:
In CommonMark, a numbered list marker followed by a heading syntax—such as "1. Heading" followed by a setext underline ("---")—is interpreted as a list item containing a paragraph, rather than a heading [1]. According to the CommonMark specification, a line starting with a numbered list marker (e.g., "1.") is parsed as a list item [2][3]. Because the specification prioritizes list item interpretation in this context, the subsequent text and the underline are treated as the content of that list item [1]. Consequently, the "1. Heading" and the following "---" line will be rendered as a list containing a paragraph, not as a level 1 heading [1]. To avoid this interpretation and force the text to be treated as a heading, you must escape the period in the numbered list marker (e.g., "1. Heading") [1]. This prevents the parser from identifying the line as a list item, allowing the subsequent text and underline to be parsed as a setext heading [1]. For ATX headings (using "#"), placing a "#" after a list marker (e.g., "1. # Heading") generally parses the "# Heading" as the content of the list item, not as a heading block, because headings cannot typically interrupt a list item in that manner within the CommonMark specification [2][3].
Citations:
- 1: https://talk.commonmark.org/t/issues-resolved-for-1-0-release/2137
- 2: https://github.com/commonmark/commonmark-spec/blob/master/spec.txt
- 3: https://raw.githubusercontent.com/commonmark/commonmark-spec/0.31.2/spec.txt
Move the section numbers inside the H3 headings.
1. ### and 2. ### are parsed as ordered-list items containing heading-like text, so the numbered sections become list content instead of document-level H3 headings. Use ### 1. Install ... and ### 2. Install ..., or plain H3 headings.
Proposed fix
-1. ### Install **pyvips** for Efficient Image Processing
+### 1. Install **pyvips** for Efficient Image Processing
-2. ### Install the Core Quality Control Library
+### 2. Install the Core Quality Control LibraryAlso applies to: 21-21
🤖 Prompt for 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.
In `@docs/getting-started/installation.md` at 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.
2 participants
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