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
More reviews will be available in 58 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (22)
✨ Finishing Touches🧪 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
TomasBalak
force-pushed
the
refactor/typing-and-api
branch
from
689becd to
7961a57
Compare
1 participant
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
None