fix: Fix rendering of alternative representation if there are more components in the alternative representation than in ROI #1240
Conversation
Reviewer's Guide by SourceryThe pull request fixes an issue where the alternative representation in ROI rendering would fail if it contained more components than the original ROI. This was achieved by introducing a method to retrieve the number of components in the alternative representation and using that value when determining the color mapping. Sequence diagram for ROI component color mappingsequenceDiagram
participant NapariView
participant ROIInfo
NapariView->>ROIInfo: get_components_num(roi_alternative_selection)
alt roi_alternative_selection is 'ROI' or not in alternative
ROIInfo-->>NapariView: max(bound_info)
else has alternative representation
ROIInfo-->>NapariView: max value from alternative representation
end
Note over NapariView: Map colors to components<br/>using returned count
Class diagram showing ROIInfo changesclassDiagram
class ROIInfo {
-roi
-bound_info
-sizes
-_alternative_component_size: dict
+fit_to_image(image: Image) ROIInfo
+get_components_num(name: str) int
+__str__() str
}
note for ROIInfo "Added _alternative_component_size cache
and get_components_num method"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe changes modify how ROI component counts are determined in the application. In the Changes
Sequence Diagram(s)sequenceDiagram
participant IV as ImageView
participant II as ImageInfo
participant RI as ROIInfo
IV->>II: get_roi_view_parameters(image_info)
II->>RI: get_components_num(roi_alternative_selection)
alt Component count calculated
RI-->>II: computed component count
else Cached value exists
RI-->>II: cached component count
end
II-->>IV: return ROI view parameters with color mapping based on count
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 300000ms (13)
🔇 Additional comments (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Czaki - I've reviewed your changes - here's some feedback:
Overall Comments:
- In the new get_components_num method, consider adding a docstring explaining the purpose and expected behavior, especially regarding when it falls back to using bound_info and caching the alternative component size.
- Ensure that self.alternative[name] is non-empty before using np.max on it. If there's a possibility it might be empty, add a validation or a default behavior to prevent runtime errors.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if name == "ROI" or name not in self.alternative: | ||
| return max(self.bound_info) | ||
| if name not in self._alternative_component_size: | ||
| self._alternative_component_size[name] = np.max(self.alternative[name]) |
There was a problem hiding this comment.
issue: Consider handling the case where self.alternative[name] is empty to avoid ValueError from np.max
np.max will raise a ValueError on empty arrays. Consider returning 0 for empty arrays or adding a check using if self.alternative[name].size > 0.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package/PartSegCore/roi_info.py (1)
74-79: Add docstring to document the method's purpose and parameters.The implementation is clean and efficient with good use of caching. Consider adding a docstring to document:
- Purpose of the method
nameparameter and its expected values- Return value and its meaning
def get_components_num(self, name): + """ + Get the number of components for a given representation. + + Args: + name (str): Name of the representation. Use "ROI" for the main ROI, + or the name of an alternative representation. + + Returns: + int: Maximum component number in the specified representation. + """ if name == "ROI" or name not in self.alternative: return max(self.bound_info) if name not in self._alternative_component_size: self._alternative_component_size[name] = np.max(self.alternative[name]) return self._alternative_component_size[name]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/PartSeg/common_gui/napari_image_view.py(1 hunks)package/PartSegCore/roi_info.py(2 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (27)
- GitHub Check: 4DNucleome.PartSeg (Tests_other test windows)
- GitHub Check: 4DNucleome.PartSeg (Tests_other test macos)
- GitHub Check: 4DNucleome.PartSeg (Tests_linux test_linux)
- GitHub Check: 4DNucleome.PartSeg (Builds pyinstaller windows)
- GitHub Check: 4DNucleome.PartSeg (Builds pyinstaller macos_arm)
- GitHub Check: 4DNucleome.PartSeg (Builds pyinstaller macos)
- GitHub Check: 4DNucleome.PartSeg (Builds sdist)
- GitHub Check: 4DNucleome.PartSeg (Builds pyinstaller_linux)
- GitHub Check: Base py3.9 / ubuntu-24.04 py 3.9 latest PyQt5
- GitHub Check: Base py3.12 / ubuntu-24.04 py 3.12 latest PyQt5
- GitHub Check: Base py3.12 / ubuntu-22.04 py 3.12 latest PyQt6
- GitHub Check: Base py3.12 / macos-14 py 3.12 latest PyQt6
- GitHub Check: Base py3.11 / windows-latest py 3.11 latest PyQt5
- GitHub Check: Base py3.11 / ubuntu-24.04 py 3.11 latest PyQt5
- GitHub Check: Base py3.11 / macos-13 py 3.11 latest PyQt5
- GitHub Check: Base py3.10 / ubuntu-24.04 py 3.10 latest PySide6
- GitHub Check: Base py3.10 / ubuntu-24.04 py 3.10 latest PyQt5
- GitHub Check: Base py3.10 / ubuntu-22.04 py 3.10 latest PyQt5 _pydantic_1
- GitHub Check: Base py3.10 / ubuntu-20.04 py 3.10 latest PySide2
- GitHub Check: Test PartSeg minimal / ubuntu-24.04 py 3.9 latest PyQt5
- GitHub Check: test_coverage / ubuntu-24.04 py 3.12 latest PyQt5
- GitHub Check: Test PartSeg conda
- GitHub Check: 4DNucleome.PartSeg (formatting_check check_formating)
- GitHub Check: 4DNucleome.PartSeg (Documentation_check help)
- GitHub Check: 4DNucleome.PartSeg (Documentation_check Notebook_check)
- GitHub Check: 4DNucleome.PartSeg (GetTestData linux)
- GitHub Check: 4DNucleome.PartSeg (manifest_check manifest_check)
🔇 Additional comments (2)
package/PartSegCore/roi_info.py (1)
65-65: LGTM!Good practice to initialize the cache dictionary in
__init__.package/PartSeg/common_gui/napari_image_view.py (1)
486-489: LGTM!The change correctly uses the dynamic component count from ROIInfo, fixing the rendering issue when alternative representations have more components than the main ROI.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1240 +/- ##
========================================
Coverage 93.14% 93.15%
========================================
Files 210 210
Lines 33268 33288 +20
========================================
+ Hits 30989 31009 +20
Misses 2279 2279 ☔ View full report in Codecov by Sentry. |
Czaki
changed the title
|
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
New Features
Bug Fixes