feat: Add option to put center of components in measurement#12
Open
Czaki wants to merge 6 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a CenterCoordinate measurement (enums and parameter model), migrates several measurements to BaseModel arguments, registers the new measurement in package init, updates pyproject dependencies (adds sympy and local-migrator and a dev group), and adds ChangesCenter Coordinate Measurement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![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
🧹 Nitpick comments (2)
src/PartSeg_smfish/measurement.py (2)
16-16: 💤 Low valueUnused import
return_none.
return_noneis imported but never referenced in the visible code. This will also trip ruffF401once linting runs.♻️ Proposed cleanup
-from toolz.functoolz import return_none🤖 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 `@src/PartSeg_smfish/measurement.py` at line 16, The import return_none from toolz.functoolz is unused in measurement.py; remove the line "from toolz.functoolz import return_none" (or replace it with the correct symbol if it was meant to be used) to eliminate the unused-import lint error (ruff F401) and keep the module imports minimal; search for any intended use of return_none in functions within measurement.py (e.g., any references to return_none) before deleting to ensure you aren't removing needed functionality.
22-22: 💤 Low valueConsider extracting a shared sentinel class for clarity.
Using an empty
BaseModelfor measurements with no parameters follows the PartSeg ecosystem convention, but extracting a sentinel likeclass _NoParams(BaseModel): passwould make the intent explicit and improve code grep-ability. This is a minor code clarity improvement.Also applies to: 47-47, 92-92
🤖 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 `@src/PartSeg_smfish/measurement.py` at line 22, Extract an explicit sentinel argument class instead of using BaseModel directly: add a small sentinel class name like _NoParams that subclasses BaseModel (class _NoParams(BaseModel): pass) and replace assignments of __argument_class__ = BaseModel with __argument_class__ = _NoParams in the measurement classes that currently use BaseModel directly; update any imports/comments as needed so references to __argument_class__ in functions/methods (e.g., measurement class definitions) now point to _NoParams to make intent and grep-ability explicit.
🤖 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 `@src/PartSeg_smfish/measurement.py`:
- Around line 145-153: DimensionName currently encodes numpy axis positions with
negative ints (X=-1, Y=-2, Z=-3) which couples the enum to array dimensionality
and causes IndexError for 2D images where result_center[dimension.value] is used
in calculate_property; change DimensionName to simple names/neutral values
(e.g., auto or 0/1/2) and update calculate_property to map DimensionName members
to safe axis indices based on the shape (e.g., lookup {DimensionName.X: -1 or
axis_for(X)} or compute axis by counting trailing axes) rather than relying on
the enum numeric value; also rename CenterType members to PEP8 UPPER_SNAKE_CASE
(e.g., GEOMETRICAL, MASS) or switch to a StrEnum with explicit display labels,
and update all usages of CenterType (e.g., in calculate_property and any UI
label generation) to use the new names/labels.
- Around line 170-191: In CenterCoordinate.calculate_property: remove the debug
print (replace with logger.debug if needed), change the bounds_info annotation
to a mapping type (e.g., dict[int, BoundInfo] or Mapping[int, BoundInfo]) so
indexing with _component_num is correct, consolidate the two branches by
selecting the mass image into a single variable and call af.density_mass_center
once (do not wrap its result in np.array([...]) and avoid the unnecessary [0]
extraction), and guard dimension indexing by mapping DimensionName to a positive
index based on result_center.size (or raise a clear error for unsupported
dimensions) instead of directly using dimension.value which can be negative for
2D images; also add a get_starting_leaf override on the CenterCoordinate class
returning area=AreaType.ROI, per_component=PerComponent.Yes to ensure
per-component outputs.
---
Nitpick comments:
In `@src/PartSeg_smfish/measurement.py`:
- Line 16: The import return_none from toolz.functoolz is unused in
measurement.py; remove the line "from toolz.functoolz import return_none" (or
replace it with the correct symbol if it was meant to be used) to eliminate the
unused-import lint error (ruff F401) and keep the module imports minimal; search
for any intended use of return_none in functions within measurement.py (e.g.,
any references to return_none) before deleting to ensure you aren't removing
needed functionality.
- Line 22: Extract an explicit sentinel argument class instead of using
BaseModel directly: add a small sentinel class name like _NoParams that
subclasses BaseModel (class _NoParams(BaseModel): pass) and replace assignments
of __argument_class__ = BaseModel with __argument_class__ = _NoParams in the
measurement classes that currently use BaseModel directly; update any
imports/comments as needed so references to __argument_class__ in
functions/methods (e.g., measurement class definitions) now point to _NoParams
to make intent and grep-ability explicit.
🪄 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
Run ID: 25f1d6e8-c42b-4cfb-ac6c-59cae4355698
📒 Files selected for processing (4)
.gitignorepyproject.tomlsrc/PartSeg_smfish/__init__.pysrc/PartSeg_smfish/measurement.py
Comment on lines
+145
to
+153
| class DimensionName(Enum): | ||
| X = -1 | ||
| Y = -2 | ||
| Z = -3 | ||
|
|
||
|
|
||
| class CenterType(Enum): | ||
| Geometrical_center = 1 | ||
| Mass_center = 2 |
There was a problem hiding this comment.
Reconsider enum value/naming choices.
Two concerns:
DimensionNameuses negative integers (X=-1, Y=-2, Z=-3) so the enum value doubles as a numpy axis index. This couples the enum to the assumption that the indexed array has at least 3 dimensions (see Line 191result_center[dimension.value]— for 2D imagesZresolves to index-3and raisesIndexError). Prefer decoupling the wire value from the indexing logic, e.g. map names to indices explicitly insidecalculate_property.CenterType.Geometrical_center/Mass_centerviolate PEP 8 enum-member convention (UPPER_SNAKE_CASE) and will likely render awkwardly in the UI. PartSeg typically derives display labels from the member name; considerGEOMETRICAL/MASS(or use aStrEnumwith explicit display labels).
🤖 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 `@src/PartSeg_smfish/measurement.py` around lines 145 - 153, DimensionName
currently encodes numpy axis positions with negative ints (X=-1, Y=-2, Z=-3)
which couples the enum to array dimensionality and causes IndexError for 2D
images where result_center[dimension.value] is used in calculate_property;
change DimensionName to simple names/neutral values (e.g., auto or 0/1/2) and
update calculate_property to map DimensionName members to safe axis indices
based on the shape (e.g., lookup {DimensionName.X: -1 or axis_for(X)} or compute
axis by counting trailing axes) rather than relying on the enum numeric value;
also rename CenterType members to PEP8 UPPER_SNAKE_CASE (e.g., GEOMETRICAL,
MASS) or switch to a StrEnum with explicit display labels, and update all usages
of CenterType (e.g., in calculate_property and any UI label generation) to use
the new names/labels.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
60-64:⚠️ Potential issue | 🟠 MajorPEP 735
[dependency-groups]needs installer support; setuptools 71+ alone doesn’t guarantee it.
pyproject.tomlrequiressetuptools>=71.0.0, which matches the setuptools-side requirement, but the installer matters for consuming the groups: in this environmentpip 23.0.1’s help does not mention--group, so installing thedevgroup via PEP 735 may fail unless CI/dev uses a PEP 735-capable installer/tool.🤖 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 `@pyproject.toml` around lines 60 - 64, The pyproject currently uses the PEP 735 section [dependency-groups] with group "dev" containing "PyQt6" and "PyOpenGL-accelerate", but many installers (current pip in CI) don't support --group; either switch to the widely supported PEP 621 pattern by moving the entries into project.optional-dependencies under a "dev" key, or ensure CI/dev environments install with an installer/version that implements PEP 735 (update pip/installer in CI and document requirement) so the "dev" dependency-group is consumable.
🤖 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 `@pyproject.toml`:
- Around line 34-35: Update pyproject.toml to pin the two bare dependencies to
safe version ranges: add appropriate lower and upper bounds for "local-migrator"
(used via from local_migrator import update_argument in segmentation.py) and for
"sympy" (used in measurement.py) so installs are reproducible and
forward-compatible; e.g. set a minimum known-working version and a conservative
upper bound (for example >=0.1.10,<1.0.0 for local-migrator and >=1.10.0,<2.0.0
for sympy) or choose ranges that match your supported compatibility matrix.
---
Outside diff comments:
In `@pyproject.toml`:
- Around line 60-64: The pyproject currently uses the PEP 735 section
[dependency-groups] with group "dev" containing "PyQt6" and
"PyOpenGL-accelerate", but many installers (current pip in CI) don't support
--group; either switch to the widely supported PEP 621 pattern by moving the
entries into project.optional-dependencies under a "dev" key, or ensure CI/dev
environments install with an installer/version that implements PEP 735 (update
pip/installer in CI and document requirement) so the "dev" dependency-group is
consumable.
🪄 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
Run ID: 0bfa0d18-bc04-4335-865a-51e20a0cad71
📒 Files selected for processing (3)
pyproject.tomlsrc/PartSeg_smfish/measurement.pysrc/PartSeg_smfish/segmentation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PartSeg_smfish/measurement.py
Comment on lines
+34
to
+35
| "sympy", | ||
| "local-migrator" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify local-migrator and sympy package existence and check for version recommendations
# Check if local-migrator exists on PyPI
echo "=== Checking local-migrator on PyPI ==="
curl -s https://pypi.org/pypi/local-migrator/json | jq -e '.info.version' || echo "Package not found on PyPI"
# Check sympy latest version
echo -e "\n=== Checking sympy latest version ==="
curl -s https://pypi.org/pypi/sympy/json | jq '.info.version'
# Search for local-migrator in the repository
echo -e "\n=== Searching for local-migrator in repository ==="
fd -t f -e py | xargs rg -l 'local.migrator|local_migrator' | head -20Repository: 4DNucleome/PartSeg-smfish
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant section of pyproject.toml around lines 25-80
echo "=== pyproject.toml (lines 20-90) ==="
nl -ba pyproject.toml | sed -n '20,90p'
# Show import usage in measurement.py and segmentation.py if present
echo -e "\n=== src/PartSeg_smfish/measurement.py (imports + sympy usage) ==="
if [ -f src/PartSeg_smfish/measurement.py ]; then
nl -ba src/PartSeg_smfish/measurement.py | rg -n "sympy|symbols|Symbol|import" | head -50
# Show around first sympy import line
line=$(nl -ba src/PartSeg_smfish/measurement.py | rg -n "from sympy" | head -1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-5)); end=$((line+20))
nl -ba src/PartSeg_smfish/measurement.py | sed -n "${start},${end}p"
fi
fi
echo -e "\n=== src/PartSeg_smfish/segmentation.py (imports + local_migrator usage) ==="
if [ -f src/PartSeg_smfish/segmentation.py ]; then
nl -ba src/PartSeg_smfish/segmentation.py | rg -n "local_migrator|local\.migrator|update_argument" | head -50
line=$(nl -ba src/PartSeg_smfish/segmentation.py | rg -n "from local_migrator" | head -1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-5)); end=$((line+20))
nl -ba src/PartSeg_smfish/segmentation.py | sed -n "${start},${end}p"
fi
fiRepository: 4DNucleome/PartSeg-smfish
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pyproject.toml (lines 1-110) ==="
nl -ba pyproject.toml | sed -n '1,110p'
echo -e "\n=== src/PartSeg_smfish/measurement.py: sympy import/usage ==="
if [ -f src/PartSeg_smfish/measurement.py ]; then
nl -ba src/PartSeg_smfish/measurement.py | rg -n "from sympy|import sympy|symbols\(|Symbol\(" | head -80
fi
echo -e "\n=== src/PartSeg_smfish/segmentation.py: local_migrator import/usage ==="
if [ -f src/PartSeg_smfish/segmentation.py ]; then
nl -ba src/PartSeg_smfish/segmentation.py | rg -n "from local_migrator|local_migrator|local\.migrator|update_argument" | head -80
fiRepository: 4DNucleome/PartSeg-smfish
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pyproject.toml (lines 20-90) ==="
sed -n '20,90p' pyproject.toml | awk '{printf "%4d\t%s\n", NR+19, $0}'
echo -e "\n=== pyproject.toml (lines 1-110) ==="
sed -n '1,110p' pyproject.toml | awk '{printf "%4d\t%s\n", NR, $0}'
echo -e "\n=== src/PartSeg_smfish/measurement.py: sympy import/usage (matches) ==="
rg -n "from sympy|import sympy|symbols\(|Symbol\(" src/PartSeg_smfish/measurement.py || true
echo -e "\n=== src/PartSeg_smfish/segmentation.py: local_migrator import/usage (matches) ==="
rg -n "from local_migrator|local_migrator|local\.migrator|update_argument" src/PartSeg_smfish/segmentation.py || trueRepository: 4DNucleome/PartSeg-smfish
Length of output: 5595
Treat local-migrator as a valid dependency; add version constraints for stability
local-migrator exists on PyPI (latest 0.1.10) and is imported via from local_migrator import update_argument in src/PartSeg_smfish/segmentation.py. sympy is also imported/used in src/PartSeg_smfish/measurement.py. Both are listed in pyproject.toml without version constraints; add lower/upper bounds for reproducible, forward-compatible installs.
🤖 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 `@pyproject.toml` around lines 34 - 35, Update pyproject.toml to pin the two
bare dependencies to safe version ranges: add appropriate lower and upper bounds
for "local-migrator" (used via from local_migrator import update_argument in
segmentation.py) and for "sympy" (used in measurement.py) so installs are
reproducible and forward-compatible; e.g. set a minimum known-working version
and a conservative upper bound (for example >=0.1.10,<1.0.0 for local-migrator
and >=1.10.0,<2.0.0 for sympy) or choose ranges that match your supported
compatibility matrix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Chores