feat: Add measurement of distance form nucleus center and border#8
feat: Add measurement of distance form nucleus center and border#8
Conversation
WalkthroughTwo new measurement classes, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MeasurementSystem
participant DistanceToNucleusCenter
participant DistanceFromNucleusBorder
participant ROI
participant Nucleus
User->>MeasurementSystem: Request measurement (e.g., DistanceToNucleusCenter)
MeasurementSystem->>DistanceToNucleusCenter: Invoke calculation
DistanceToNucleusCenter->>ROI: Get ROI center
DistanceToNucleusCenter->>Nucleus: Get nucleus center
DistanceToNucleusCenter->>MeasurementSystem: Return distance
User->>MeasurementSystem: Request measurement (DistanceFromNucleusBorder)
MeasurementSystem->>DistanceFromNucleusBorder: Invoke calculation
DistanceFromNucleusBorder->>ROI: Get ROI center
DistanceFromNucleusBorder->>Nucleus: Get nucleus border points
DistanceFromNucleusBorder->>MeasurementSystem: Return min distance
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/PartSeg_smfish/__init__.py(1 hunks)src/PartSeg_smfish/measurement.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/PartSeg_smfish/__init__.py (1)
src/PartSeg_smfish/measurement.py (2)
DistanceToNucleusCenter(37-81)DistanceFromNucleusBorder(84-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: macos-latest py3.12
- GitHub Check: windows-latest py3.11
- GitHub Check: ubuntu-latest py3.12
- GitHub Check: macos-latest py3.10
- GitHub Check: windows-latest py3.12
- GitHub Check: ubuntu-latest py3.10
- GitHub Check: ubuntu-latest py3.11
- GitHub Check: windows-latest py3.10
🔇 Additional comments (2)
src/PartSeg_smfish/measurement.py (1)
9-10: LGTM: New imports are properly added.The imports for
get_borderandsymbolsare correctly added to support the new measurement classes.src/PartSeg_smfish/__init__.py (1)
29-36: LGTM: New measurement classes are properly registered.The two new measurement classes are correctly registered under the
analysis_measurementcategory, following the same pattern as the existingComponentTypemeasurement. This will integrate them into the measurement framework without affecting existing functionality.
| class DistanceToNucleusCenter(MeasurementMethodBase): | ||
| text_info = ( | ||
| "Distance to nucleus center", | ||
| "Distance to nucleus center in units", | ||
| ) | ||
|
|
||
| @classmethod | ||
| def get_fields(cls): | ||
| return [] | ||
|
|
||
| @classmethod | ||
| def need_full_data(cls): | ||
| return True | ||
|
|
||
| @classmethod | ||
| def get_starting_leaf(cls): | ||
| return Leaf( | ||
| name=cls.text_info[0], | ||
| area=AreaType.ROI, | ||
| per_component=PerComponent.Yes, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def calculate_property( | ||
| area_array, | ||
| roi_alternative: dict[str, np.ndarray], | ||
| help_dict: dict, | ||
| result_scalar: float, | ||
| voxel_size: tuple[float, float, float], | ||
| **kwargs, | ||
| ): | ||
| if "nucleus_center" not in help_dict: | ||
| nucleus = roi_alternative["nucleus"] | ||
| help_dict["nucleus_center"] = ( | ||
| af.density_mass_center(nucleus, voxel_size) * result_scalar | ||
| ) | ||
| nucleus_center = help_dict["nucleus_center"] | ||
| roi_center = ( | ||
| af.density_mass_center(area_array, voxel_size) * result_scalar | ||
| ) | ||
| return np.sqrt(np.sum((nucleus_center - roi_center) ** 2)) | ||
|
|
||
| @classmethod | ||
| def get_units(cls, ndim): | ||
| return symbols("{}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the units method to return proper units.
The DistanceToNucleusCenter class implementation is correct for calculating Euclidean distance with good caching optimization. However, the get_units method returns an empty symbolic unit which likely isn't intended.
@classmethod
def get_units(cls, ndim):
- return symbols("{}")
+ return symbols("μm") # or appropriate unit symbolThe distance calculation logic is sound - it computes the Euclidean distance between the density mass centers of the ROI and nucleus, with proper scaling by result_scalar.
📝 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.
| class DistanceToNucleusCenter(MeasurementMethodBase): | |
| text_info = ( | |
| "Distance to nucleus center", | |
| "Distance to nucleus center in units", | |
| ) | |
| @classmethod | |
| def get_fields(cls): | |
| return [] | |
| @classmethod | |
| def need_full_data(cls): | |
| return True | |
| @classmethod | |
| def get_starting_leaf(cls): | |
| return Leaf( | |
| name=cls.text_info[0], | |
| area=AreaType.ROI, | |
| per_component=PerComponent.Yes, | |
| ) | |
| @staticmethod | |
| def calculate_property( | |
| area_array, | |
| roi_alternative: dict[str, np.ndarray], | |
| help_dict: dict, | |
| result_scalar: float, | |
| voxel_size: tuple[float, float, float], | |
| **kwargs, | |
| ): | |
| if "nucleus_center" not in help_dict: | |
| nucleus = roi_alternative["nucleus"] | |
| help_dict["nucleus_center"] = ( | |
| af.density_mass_center(nucleus, voxel_size) * result_scalar | |
| ) | |
| nucleus_center = help_dict["nucleus_center"] | |
| roi_center = ( | |
| af.density_mass_center(area_array, voxel_size) * result_scalar | |
| ) | |
| return np.sqrt(np.sum((nucleus_center - roi_center) ** 2)) | |
| @classmethod | |
| def get_units(cls, ndim): | |
| return symbols("{}") | |
| @classmethod | |
| def get_units(cls, ndim): | |
| - return symbols("{}") | |
| + return symbols("μm") # or appropriate unit symbol |
🤖 Prompt for AI Agents
In src/PartSeg_smfish/measurement.py between lines 37 and 82, the get_units
method in the DistanceToNucleusCenter class currently returns an empty symbolic
unit which is incorrect. Update this method to return the appropriate unit for
distance measurement, such as a length unit symbol, to correctly represent the
units of the calculated distance.
There was a problem hiding this comment.
The unit is determined by result_scalar and the proper text value is set using str.format when printing reesult.
There was a problem hiding this comment.
@Czaki Thank you for the clarification! You're absolutely right - I misunderstood how the unit system works in this codebase. The symbols("{}") creates a placeholder that gets filled in later using str.format when the results are printed, with the actual unit determined by the result_scalar. This is a much more flexible approach than hardcoding the unit symbol.
I appreciate you taking the time to explain this implementation detail!
✏️ Learnings added
Learnt from: Czaki
PR: 4DNucleome/PartSeg-smfish#8
File: src/PartSeg_smfish/measurement.py:37-82
Timestamp: 2025-07-09T23:03:57.430Z
Learning: In PartSeg measurement classes, the `get_units()` method returns `symbols("{}")` as a placeholder that gets filled in later using `str.format` when printing results. The actual unit is determined by the `result_scalar` parameter, making the unit system dynamic and flexible.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| class DistanceFromNucleusBorder(MeasurementMethodBase): | ||
| text_info = ( | ||
| "Distance from nucleus border", | ||
| "Distance from nucleus border in units", | ||
| ) | ||
|
|
||
| @classmethod | ||
| def get_fields(cls): | ||
| return [] | ||
|
|
||
| @classmethod | ||
| def need_full_data(cls): | ||
| return True | ||
|
|
||
| @classmethod | ||
| def get_starting_leaf(cls): | ||
| return Leaf( | ||
| name=cls.text_info[0], | ||
| area=AreaType.ROI, | ||
| per_component=PerComponent.Yes, | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def calculate_property( | ||
| area_array, | ||
| roi_alternative: dict[str, np.ndarray], | ||
| help_dict: dict, | ||
| result_scalar: float, | ||
| roi_annotation: dict, | ||
| _component_num: int, | ||
| voxel_size: tuple[float, float, float], | ||
| **kwargs, | ||
| ): | ||
| if "nucleus_border" not in help_dict: | ||
| nucleus = roi_alternative["nucleus"] | ||
| area_pos = np.transpose(np.nonzero(get_border(nucleus))).astype( | ||
| float | ||
| ) | ||
| area_pos += 0.5 | ||
| for i, val in enumerate( | ||
| (x * result_scalar for x in reversed(voxel_size)), start=1 | ||
| ): | ||
| area_pos[:, -i] *= val | ||
| help_dict["nucleus_border"] = area_pos | ||
|
|
||
| nucleus_border = help_dict["nucleus_border"] | ||
| roi_center = ( | ||
| af.density_mass_center(area_array, voxel_size) * result_scalar | ||
| ) | ||
| res = np.sqrt( | ||
| np.min(np.sum((nucleus_border - roi_center) ** 2, axis=1)) | ||
| ) | ||
| if roi_annotation[_component_num].get("type") == "Nucleus": | ||
| return -res | ||
| return res | ||
|
|
||
| @classmethod | ||
| def get_units(cls, ndim): | ||
| return symbols("{}") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix the units method and verify coordinate scaling logic.
The DistanceFromNucleusBorder class implements a complex but correct distance calculation. The coordinate scaling logic appears sound, but the units issue needs fixing.
@classmethod
def get_units(cls, ndim):
- return symbols("{}")
+ return symbols("μm") # or appropriate unit symbolThe implementation correctly:
- Caches nucleus border coordinates for efficiency
- Scales coordinates by voxel size and result_scalar
- Finds minimum distance to border points
- Returns negative distance for "Nucleus" ROI type (indicating inside the nucleus)
Please verify that the coordinate scaling logic in lines 123-126 correctly handles the dimensionality and ordering:
🏁 Script executed:
#!/bin/bash
# Verify the coordinate scaling logic matches the expected dimensionality
ast-grep --pattern 'for i, val in enumerate($$$):
area_pos[:, -i] *= val'Length of output: 397
🏁 Script executed:
#!/bin/bash
head -n 40 src/PartSeg_smfish/measurement.pyLength of output: 1062
🏁 Script executed:
#!/bin/bash
rg -n "def get_units" src/PartSeg_smfish/measurement.py -B3 -A3Length of output: 500
Fix the units for DistanceFromNucleusBorder
The coordinate-scaling loop (Lines 123–126) correctly applies voxel_size and result_scalar to each axis in reverse order. Only the get_units method needs updating to reflect a physical length unit.
File: src/PartSeg_smfish/measurement.py
@@ class DistanceFromNucleusBorder(MeasurementMethodBase):
@classmethod
def get_units(cls, ndim):
- return symbols("{}")
+ # Return the physical length unit, e.g., micrometers
+ return symbols("µm")📝 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.
| class DistanceFromNucleusBorder(MeasurementMethodBase): | |
| text_info = ( | |
| "Distance from nucleus border", | |
| "Distance from nucleus border in units", | |
| ) | |
| @classmethod | |
| def get_fields(cls): | |
| return [] | |
| @classmethod | |
| def need_full_data(cls): | |
| return True | |
| @classmethod | |
| def get_starting_leaf(cls): | |
| return Leaf( | |
| name=cls.text_info[0], | |
| area=AreaType.ROI, | |
| per_component=PerComponent.Yes, | |
| ) | |
| @staticmethod | |
| def calculate_property( | |
| area_array, | |
| roi_alternative: dict[str, np.ndarray], | |
| help_dict: dict, | |
| result_scalar: float, | |
| roi_annotation: dict, | |
| _component_num: int, | |
| voxel_size: tuple[float, float, float], | |
| **kwargs, | |
| ): | |
| if "nucleus_border" not in help_dict: | |
| nucleus = roi_alternative["nucleus"] | |
| area_pos = np.transpose(np.nonzero(get_border(nucleus))).astype( | |
| float | |
| ) | |
| area_pos += 0.5 | |
| for i, val in enumerate( | |
| (x * result_scalar for x in reversed(voxel_size)), start=1 | |
| ): | |
| area_pos[:, -i] *= val | |
| help_dict["nucleus_border"] = area_pos | |
| nucleus_border = help_dict["nucleus_border"] | |
| roi_center = ( | |
| af.density_mass_center(area_array, voxel_size) * result_scalar | |
| ) | |
| res = np.sqrt( | |
| np.min(np.sum((nucleus_border - roi_center) ** 2, axis=1)) | |
| ) | |
| if roi_annotation[_component_num].get("type") == "Nucleus": | |
| return -res | |
| return res | |
| @classmethod | |
| def get_units(cls, ndim): | |
| return symbols("{}") | |
| @classmethod | |
| def get_units(cls, ndim): | |
| - return symbols("{}") | |
| + # Return the physical length unit, e.g., micrometers | |
| + return symbols("µm") |
🤖 Prompt for AI Agents
In src/PartSeg_smfish/measurement.py around lines 84 to 142, the get_units
method currently returns an empty symbol string, which does not reflect the
physical length units used in the distance calculation. Update the get_units
method to return the appropriate physical length unit symbol, such as "µm" or
"um", to correctly represent the units of the distance from the nucleus border.
1 participant
Summary by CodeRabbit