[MSD-443][feat] 3D correlation improvements#3437
Open
tmoerkerken wants to merge 1 commit into
Open
Conversation
tmoerkerken
force-pushed
the
use-com-for-fm-refinement
branch
from
c28a63e to
2f6a326
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the targeting refinement system in the multi-point correlation GUI from a Z-only refinement to a full 3D XYZ refinement using center-of-mass computation. Two image utilities (brightest-channel selection and COM computation) and unit tests were added. The 3D coordinate conversion helpers were renamed to snake_case and all call sites updated. The UI replaces the "Refine Z" button with a "Refine" XYZ-targeting button and new state/auto-trigger logic. A separate bug fix corrects a save_project call to use tab_data.main. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/odemis/gui/cont/acquisition/cryo_acq.py (1)
214-219: Dead code:tdct_availableis computed but never used.The variable
tdct_availableis computed on lines 214-218 but line 219 unconditionally enables the button withEnable(True), making the computation unused. Either remove the dead code or use the variable as intended.♻️ Option 1: Remove dead code if unconditional enable is intended
def _check_correlation_controls(self, current_feature: CryoFeature): """ Enable or disable the correlation controls :param current_feature: the current feature """ - # Enable the correlation controls if the current feature has a reference FIB image and altleast one FM Z stack - tdct_available = ( - current_feature is not None - and current_feature.reference_image is not None - and any(isinstance(s, StaticFluoStream) and hasattr(s, "zIndex") for s in current_feature.streams.value) - ) self._panel.btn_tdct.Enable(True)♻️ Option 2: Use `tdct_available` if conditional enable was intended
- self._panel.btn_tdct.Enable(True) + self._panel.btn_tdct.Enable(tdct_available)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/acquisition/cryo_acq.py` around lines 214 - 219, The local variable tdct_available is computed but never used; either remove its computation or use it to control the button enablement. Locate the expression building tdct_available (which checks current_feature, current_feature.reference_image, and any(isinstance(s, StaticFluoStream) and hasattr(s, "zIndex") for s in current_feature.streams.value)) and either delete that block entirely if the button should always be enabled, or change the unconditional self._panel.btn_tdct.Enable(True) call to self._panel.btn_tdct.Enable(tdct_available) so the button state reflects the computed condition.src/odemis/util/test/img_test.py (1)
2356-2379: Test may not reflect intended usage pattern.The test passes the full
sub_image(shape 10×20×20) directly tocompute_local_center_of_mass, but the function's design suggests it expects an already-extracted sub-region:
get_bounding_box_slicereturns a slice for extraction- The extracted region should be passed to
compute_local_center_of_mass- The function then translates local coordinates back to global using the slice offsets
Current test: passes full image → COM computed on full image → offsets added to already-global coordinates → potentially double-offset.
The test passes because the assertion ranges are wide enough to accommodate both interpretations.
♻️ Suggested fix: Extract sub-image before computing COM
def test_compute_local_center_of_mass_with_noise(self): """Test COM computation correctly filters background noise.""" # Create 3D image with strong peak and weak noise sub_image = numpy.zeros((10, 20, 20)) # Add signal (strong) for z in range(4, 7): for y in range(8, 13): for x in range(8, 13): sub_image[z, y, x] = 100.0 # Add weak noise (much smaller than signal) sub_image += numpy.random.rand(10, 20, 20) * 2.0 roi = img.get_bounding_box_slice(10, 10, 5, 5, 20, 20) + extracted = sub_image[roi] # Extract the sub-region com_z, com_y, com_x = img.compute_local_center_of_mass( - sub_image, roi, baseline_percentile=95.0 + extracted, roi, baseline_percentile=95.0 ) # COM z should be near peak (z=4-6, center at 5) self.assertAlmostEqual(com_z, 5.0, delta=1.0) - # COM should be within the extracted ROI [5:16] range - self.assertGreaterEqual(com_x, 5) - self.assertLess(com_x, 16) - self.assertGreaterEqual(com_y, 5) - self.assertLess(com_y, 16) + # COM should be near the signal center (y=10, x=10 in global coords) + self.assertAlmostEqual(com_y, 10.0, delta=2.0) + self.assertAlmostEqual(com_x, 10.0, delta=2.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/test/img_test.py` around lines 2356 - 2379, The test test_compute_local_center_of_mass_with_noise is passing the full sub_image to compute_local_center_of_mass instead of the extracted ROI; use get_bounding_box_slice to obtain roi (already computed as roi) and then extract the local sub-image (e.g., roi_sub = sub_image[roi]) and pass that roi_sub into compute_local_center_of_mass so the function receives a local region and uses the slice offsets correctly; keep assertions the same but ensure you pass roi_sub rather than sub_image when calling compute_local_center_of_mass.src/odemis/util/img.py (1)
1479-1502: Consider documenting or handling the Z coordinate offset.The function offsets Y and X coordinates by
roi_slicestart positions to convert from local to global coordinates, but Z is returned unchanged (line 1499). This is correct ifroi_slice[0]is alwaysslice(None)(full Z range), as produced byget_bounding_box_slice. However, the comment "Global Pixel Z" may be misleading since it's actually the local sub-image Z index.Consider either:
- Clarifying in the docstring that Z is not offset (assumes full Z extraction)
- Adding Z offset for consistency:
com_sub[0] + roi_slice[0].start(would need to handleslice(None)case)📝 Clarify Z coordinate handling in docstring
- :returns: Tuple of (global_z, global_y, global_x) in pixel coordinates maintaining input order. + :returns: Tuple of (z, global_y, global_x) in pixel coordinates. + Note: Z is the local sub-image index (assumes full Z-range extraction), + while Y and X are offset to global image coordinates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/util/img.py` around lines 1479 - 1502, compute_local_center_of_mass currently returns com_sub[0] (local Z) without applying the roi_slice Z offset; update the function to add the Z offset similar to X/Y by computing z_start = roi_slice[0].start or 0 (treat slice(None) or None as 0) and return com_sub[0] + z_start, and update the docstring to state that returned (global_z, global_y, global_x) are global pixel coordinates after applying roi_slice offsets; refer to symbols compute_local_center_of_mass, roi_slice, com_sub, and baseline_percentile when making the change.src/odemis/gui/cont/multi_point_correlation.py (1)
837-837: Consider using tuple unpacking for cleaner syntax.As flagged by static analysis, tuple unpacking is more idiomatic than concatenation.
♻️ Suggested refactor
- multi_crop = raw_multi[(slice(None),) + roi] + multi_crop = raw_multi[(slice(None), *roi)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/multi_point_correlation.py` at line 837, Replace the index construction that concatenates tuples for slicing in the multi_crop assignment: instead of building the index via (slice(None),) + roi, use tuple unpacking (starred unpack) to create a cleaner slice expression; update the code around the multi_crop assignment that references raw_multi and roi to use the unpacking form so the resulting index contains slice(None) followed by the elements of roi.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Line 851: The calculation of com_pixel_z_iso uses pxs[2] directly which can
raise IndexError when MD_PIXEL_SIZE has only two elements; update the code
around com_pixel_z_iso (the line computing com_pixel_z_iso = com_pixel_z *
pxs[2] / pxs[0]) to safely handle missing Z pixel size by reading pxs with a
safe accessor or by normalizing/padding pxs (e.g., use pxs[2] if present,
otherwise fall back to pxs[0] or a default 1.0) before computing
com_pixel_z_iso; ensure the adjustment is applied wherever pxs[2] is assumed and
keep variable names pxs and com_pixel_z_iso so the change is easy to locate.
- Around line 840-851: best_c is the brightest-channel index inside the
multi-channel crop (from get_brightest_channel/compute_local_center_of_mass) but
the code uses it to index streams_projections (best_stream =
streams_projections[best_c].stream), causing a channel-vs-stream mismatch;
change the lookup to use the stream associated with the crop rather than best_c
— i.e., obtain the correct stream object (the one that produced this multi_crop,
e.g., the loop variable or a stream_index like stream_idx) and then use that
stream to fetch raw channel metadata and pxs, while still using best_c to index
the raw channel within that stream; update best_stream assignment accordingly so
best_stream = streams_projections[stream_idx].stream (or use the existing stream
variable) and keep best_channel_raw = best_stream.raw[0] /
best_stream.raw[best_c] as appropriate.
- Line 829: The assignment to raw_multi uses a hardcoded streams_projections[1]
which can raise IndexError and may pick the wrong stream; update the logic in
the multi-point correlation code that assigns raw_multi (the line creating
raw_multi = numpy.asarray(streams_projections[1].stream.raw)) to first validate
that streams_projections has the expected length and select the correct visible
stream (e.g., use streams_projections[0] if only one exists, or find the
visible/active stream by checking its attributes) or loop over
streams_projections to process each stream instead of indexing [1]; ensure you
add a guard (if not streams_projections: raise/log/return) and replace the
hardcoded index with a safe selection mechanism.
---
Nitpick comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 214-219: The local variable tdct_available is computed but never
used; either remove its computation or use it to control the button enablement.
Locate the expression building tdct_available (which checks current_feature,
current_feature.reference_image, and any(isinstance(s, StaticFluoStream) and
hasattr(s, "zIndex") for s in current_feature.streams.value)) and either delete
that block entirely if the button should always be enabled, or change the
unconditional self._panel.btn_tdct.Enable(True) call to
self._panel.btn_tdct.Enable(tdct_available) so the button state reflects the
computed condition.
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Line 837: Replace the index construction that concatenates tuples for slicing
in the multi_crop assignment: instead of building the index via (slice(None),) +
roi, use tuple unpacking (starred unpack) to create a cleaner slice expression;
update the code around the multi_crop assignment that references raw_multi and
roi to use the unpacking form so the resulting index contains slice(None)
followed by the elements of roi.
In `@src/odemis/util/img.py`:
- Around line 1479-1502: compute_local_center_of_mass currently returns
com_sub[0] (local Z) without applying the roi_slice Z offset; update the
function to add the Z offset similar to X/Y by computing z_start =
roi_slice[0].start or 0 (treat slice(None) or None as 0) and return com_sub[0] +
z_start, and update the docstring to state that returned (global_z, global_y,
global_x) are global pixel coordinates after applying roi_slice offsets; refer
to symbols compute_local_center_of_mass, roi_slice, com_sub, and
baseline_percentile when making the change.
In `@src/odemis/util/test/img_test.py`:
- Around line 2356-2379: The test test_compute_local_center_of_mass_with_noise
is passing the full sub_image to compute_local_center_of_mass instead of the
extracted ROI; use get_bounding_box_slice to obtain roi (already computed as
roi) and then extract the local sub-image (e.g., roi_sub = sub_image[roi]) and
pass that roi_sub into compute_local_center_of_mass so the function receives a
local region and uses the slice offsets correctly; keep assertions the same but
ensure you pass roi_sub rather than sub_image when calling
compute_local_center_of_mass.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36dc3c8d-35c7-4bd4-b757-990bc16530dd
📒 Files selected for processing (6)
src/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/util/img.pysrc/odemis/util/test/img_test.py
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous “Refine Z” fiducial refinement approach with a 3D center-of-mass (CoM) based targeting workflow, aiming to improve robustness to imperfect user placement and simplify the refinement algorithm.
Changes:
- Added CoM-targeting helper utilities (
get_bounding_box_slice,get_brightest_channel,compute_local_center_of_mass) toodemis.util.img. - Updated multipoint correlation GUI targeting logic to refine X/Y/Z via CoM instead of the previous Z-only Gaussian method.
- Updated the correlation dialog button label/comment and added unit tests for the new helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/odemis/util/img.py |
Adds ROI slicing, brightest-channel selection, and CoM computation helpers. |
src/odemis/util/test/img_test.py |
Adds tests covering the new helper utilities. |
src/odemis/gui/cont/multi_point_correlation.py |
Replaces the old Z-refinement path with CoM-based XYZ targeting logic. |
src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc |
Updates the targeting button label/comment (“Refine Z” → “Refine”). |
src/odemis/gui/main_xrc.py |
Mirrors the XRC label/comment change in the generated Python resource. |
Comment on lines
+828
to
+832
| # Ensure multi-channel compatibility (reshape to 4D if necessary) | ||
| raw_multi = numpy.asarray(streams_projections[1].stream.raw) | ||
| if raw_multi.ndim == 3: | ||
| raw_multi = raw_multi[numpy.newaxis, ...] | ||
|
|
There was a problem hiding this comment.
streams_projections[1] assumes at least 2 visible streams and can raise IndexError when only one FM stream is available. Also, indexing into the list like this is brittle because stream_tree.flat ordering can change (e.g., if the FIB stream is present/absent). Prefer filtering streams_projections to the relevant FM StaticFluoStream entries and then operating on that list instead of hard-coding an index.
Comment on lines
+828
to
+832
| # Ensure multi-channel compatibility (reshape to 4D if necessary) | ||
| raw_multi = numpy.asarray(streams_projections[1].stream.raw) | ||
| if raw_multi.ndim == 3: | ||
| raw_multi = raw_multi[numpy.newaxis, ...] | ||
|
|
There was a problem hiding this comment.
numpy.asarray(streams_projections[1].stream.raw) is converting stream.raw (a list of DataArray objects) into an object array rather than a numeric image stack. This will make raw_multi.ndim/raw_multi.shape[...] incorrect and will break ROI cropping. Use the underlying DataArray (e.g., stream.raw[0]) and then numpy.asarray(...) on that, or explicitly stack multiple streams into a (C, Z, Y, X) numeric array.
Suggested change
| # Ensure multi-channel compatibility (reshape to 4D if necessary) | |
| raw_multi = numpy.asarray(streams_projections[1].stream.raw) | |
| if raw_multi.ndim == 3: | |
| raw_multi = raw_multi[numpy.newaxis, ...] | |
| # Ensure multi-channel compatibility by explicitly stacking the | |
| # underlying DataArray from each visible stream into (C, Z, Y, X). | |
| raw_channels = [] | |
| for stream_projection in streams_projections: | |
| raw_channel = numpy.asarray(stream_projection.stream.raw[0]) | |
| if raw_channel.ndim == 2: | |
| raw_channel = raw_channel[numpy.newaxis, ...] | |
| raw_channels.append(raw_channel) | |
| raw_multi = numpy.stack(raw_channels, axis=0) |
Comment on lines
+839
to
+850
| # Find best channel and compute COM | ||
| best_c = get_brightest_channel(multi_crop) | ||
| com_pixel_z, com_pixel_y, com_pixel_x = compute_local_center_of_mass( | ||
| multi_crop[best_c], roi, baseline_percentile=95.0 | ||
| ) | ||
|
|
||
| # Account for iso-voxel enforcement using best channel's metadata | ||
| # Z array indices must be scaled to match the iso-voxel pixel size | ||
| best_stream = streams_projections[best_c].stream | ||
| best_channel_raw = best_stream.raw[0] | ||
| md = best_stream._find_metadata(best_channel_raw.metadata) | ||
| pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) |
There was a problem hiding this comment.
best_c is computed from multi_crop but is then used to index streams_projections for metadata (streams_projections[best_c]). This channel index will not reliably correspond to the same stream (and can even select the FIB stream if it is present as the first projection). Build a consistent mapping between the channel axis of the array you analyze and the stream (and/or channel) it came from, and use that mapping when selecting metadata.
| best_channel_raw = best_stream.raw[0] | ||
| md = best_stream._find_metadata(best_channel_raw.metadata) | ||
| pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) | ||
| com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] |
There was a problem hiding this comment.
pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) can return a 2-tuple, but com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] unconditionally indexes pxs[2], which will raise IndexError. Guard on len(pxs) >= 3 (or use a 3-element default) and decide on a safe fallback when Z spacing is unavailable.
Suggested change
| com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] | |
| if len(pxs) >= 3 and pxs[0]: | |
| com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] | |
| else: | |
| com_pixel_z_iso = com_pixel_z |
Comment on lines
+1491
to
+1494
| baseline = numpy.percentile(sub_image, baseline_percentile) | ||
| sub_image_weights = numpy.where(sub_image > baseline, sub_image - baseline, 0) | ||
| com_sub = scipy.ndimage.center_of_mass(numpy.asarray(sub_image_weights)) | ||
|
|
There was a problem hiding this comment.
If sub_image_weights sums to 0 (e.g., uniform data or an ROI with no pixels above the baseline), scipy.ndimage.center_of_mass returns (nan, nan, nan). Callers like the GUI targeting path will then write NaNs into the target coordinates. Handle the zero-weight case explicitly (e.g., return None, raise a clear exception, or fall back to the ROI center/original target).
Comment on lines
+2351
to
+2367
| multi_channel = numpy.random.rand(3, 10, 20, 20) | ||
| multi_channel[1] = numpy.ones((10, 20, 20)) * 100 # Make channel 1 bright | ||
| best_c = img.get_brightest_channel(multi_channel) | ||
| self.assertEqual(best_c, 1) | ||
|
|
||
| def test_compute_local_center_of_mass_with_noise(self): | ||
| """Test COM computation correctly filters background noise.""" | ||
| # Create 3D image with strong peak and weak noise | ||
| sub_image = numpy.zeros((10, 20, 20)) | ||
| # Add signal (strong) | ||
| for z in range(4, 7): | ||
| for y in range(8, 13): | ||
| for x in range(8, 13): | ||
| sub_image[z, y, x] = 100.0 | ||
| # Add weak noise (much smaller than signal) | ||
| sub_image += numpy.random.rand(10, 20, 20) * 2.0 | ||
|
|
There was a problem hiding this comment.
These tests rely on numpy.random without seeding. While failures may be unlikely with the current ranges, unseeded randomness can still produce flaky tests and makes failures hard to reproduce. Prefer a fixed seed via numpy.random.default_rng(seed) (or fully deterministic arrays) for both test_brightest_channel and the COM noise test.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/odemis/gui/cont/multi_point_correlation.py (2)
828-856:⚠️ Potential issue | 🔴 CriticalBuild the COM stack from the visible FM streams, not
streams_projections[1].This controller rebuilds each FM stream from
stream.raw[0], so a single projection only contributes one(Z, Y, X)volume. Pullingraw_multifromstreams_projections[1]both assumes a second visible projection exists and makesbest_cunrelated tostreams_projections; Line 847 can then select the wrong stream metadata.Proposed fix
+ fm_projections = [ + proj for proj in streams_projections + if isinstance(proj.stream, StaticFluoStream) + ] + if not fm_projections: + wx.MessageBox("FM streams are not available for refining targets", + "Error", wx.OK | wx.ICON_ERROR) + return + + reference_stream = fm_projections[0].stream - raw_multi = numpy.asarray(streams_projections[1].stream.raw) - if raw_multi.ndim == 3: - raw_multi = raw_multi[numpy.newaxis, ...] + raw_multi = numpy.stack( + [numpy.asarray(proj.stream.raw[0]) for proj in fm_projections], + axis=0, + ) @@ - best_stream = streams_projections[best_c].stream + best_stream = fm_projections[best_c].stream @@ - physical_coords = get_physical_3d_coordinates( - self.correlation_target.fm_streams[0], - (com_pixel_x, com_pixel_y, com_pixel_z_iso) - ) + physical_coords = get_physical_3d_coordinates( + reference_stream, + (com_pixel_x, com_pixel_y, com_pixel_z_iso) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/multi_point_correlation.py` around lines 828 - 856, The code currently builds raw_multi from streams_projections[1].stream.raw which wrongly assumes a second projection; instead build raw_multi by stacking the visible FM streams' first raw volume (stream.raw[0]) from self.correlation_target.fm_streams so each visible projection contributes one (Z,Y,X) volume, then proceed to get_brightest_channel, compute_local_center_of_mass and pick metadata from the corresponding fm stream; ensure best_c indexes into the same list you constructed (the stacked raw_multi) when you set best_stream and lookup metadata (replace usage of streams_projections[1] with the fm_stream stack and keep compute_local_center_of_mass, get_brightest_channel and px scaling logic unchanged).
849-851:⚠️ Potential issue | 🟠 MajorThe
MD_PIXEL_SIZEfallback cannot satisfypxs[2].If Z spacing is missing from the metadata, the current default has only two elements, so Line 851 raises
IndexErrorinstead of degrading gracefully.Proposed fix
- pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) - com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] + pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6, 1e-6)) + pxs_z = pxs[2] if len(pxs) > 2 else pxs[0] + com_pixel_z_iso = com_pixel_z * pxs_z / pxs[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/multi_point_correlation.py` around lines 849 - 851, The code assumes model.MD_PIXEL_SIZE returns three elements but the fallback (1e-6,1e-6) can be length 2 causing an IndexError when computing com_pixel_z_iso; when retrieving pxs from best_stream._find_metadata(best_channel_raw.metadata) ensure pxs has three components (z,y,x) by providing a 3-tuple default like (1e-6,1e-6,1e-6) or by explicitly padding/using pxs[2] = pxs[2] if present else pxs[0] before using it in the com_pixel_z_iso calculation so com_pixel_z and model.MD_PIXEL_SIZE usage is safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 823-826: The current call to get_pixel_3d_coordinates allows seed
points outside the FM volume; before building the ROI you must reject
out-of-bounds targets. After obtaining coords from
self._tab_data_model.main.currentTarget.value.coordinates.value and/or when
calling get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords),
validate the resulting pixel X/Y against the FM stream's pixel bounds (or call
get_pixel_3d_coordinates with check_bbox=True if available) and if X or Y is
negative or >= the stream's width/height, abort/return early (or mark the target
invalid) instead of proceeding to build the ROI; reference the variables
self._tab_data_model.main.currentTarget, get_pixel_3d_coordinates, and
self.correlation_target.fm_streams[0] when locating where to add this check.
---
Duplicate comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 828-856: The code currently builds raw_multi from
streams_projections[1].stream.raw which wrongly assumes a second projection;
instead build raw_multi by stacking the visible FM streams' first raw volume
(stream.raw[0]) from self.correlation_target.fm_streams so each visible
projection contributes one (Z,Y,X) volume, then proceed to
get_brightest_channel, compute_local_center_of_mass and pick metadata from the
corresponding fm stream; ensure best_c indexes into the same list you
constructed (the stacked raw_multi) when you set best_stream and lookup metadata
(replace usage of streams_projections[1] with the fm_stream stack and keep
compute_local_center_of_mass, get_brightest_channel and px scaling logic
unchanged).
- Around line 849-851: The code assumes model.MD_PIXEL_SIZE returns three
elements but the fallback (1e-6,1e-6) can be length 2 causing an IndexError when
computing com_pixel_z_iso; when retrieving pxs from
best_stream._find_metadata(best_channel_raw.metadata) ensure pxs has three
components (z,y,x) by providing a 3-tuple default like (1e-6,1e-6,1e-6) or by
explicitly padding/using pxs[2] = pxs[2] if present else pxs[0] before using it
in the com_pixel_z_iso calculation so com_pixel_z and model.MD_PIXEL_SIZE usage
is safe.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75f59b7b-604f-4f0c-a4ef-a87dcc828537
📒 Files selected for processing (5)
src/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/util/img.pysrc/odemis/util/test/img_test.py
✅ Files skipped from review due to trivial changes (3)
- src/odemis/gui/main_xrc.py
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
- src/odemis/util/test/img_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/util/img.py
Comment on lines
823
to
+826
| coords = self._tab_data_model.main.currentTarget.value.coordinates.value | ||
| pixel_coords = getPixel3DCoordinates(self.correlation_target.fm_streams[0], coords) | ||
| das = [interpolate_z_stack(da=stream_projection.stream.raw[0] | ||
| [:, | ||
| int(pixel_coords[1]):int(pixel_coords[1])+1, | ||
| int(pixel_coords[0]):int(pixel_coords[0])+1], | ||
| method="linear") | ||
| for stream_projection in streams_projections] | ||
|
|
||
| z = float(get_optimized_z_gauss(das, int(0), int(0), int(pixel_coords[2]))) | ||
| z_p = getPhysical3DCoordinates(self.correlation_target.fm_streams[0], | ||
| (pixel_coords[0],pixel_coords[1], z))[2] | ||
| self._tab_data_model.main.currentTarget.value.coordinates.value[2] = z_p | ||
| pixel_coords = get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords) | ||
|
|
||
| target_x, target_y = int(pixel_coords[0]), int(pixel_coords[1]) |
There was a problem hiding this comment.
Reject out-of-bounds seed points before building the ROI.
get_pixel_3d_coordinates(..., check_bbox=False) lets negative or oversized X/Y through. A target edited outside the FM volume can therefore produce an empty crop or a large unrelated crop once the ROI is built, which defeats the robustness this handler is trying to add.
Proposed fix
- pixel_coords = get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords)
-
- target_x, target_y = int(pixel_coords[0]), int(pixel_coords[1])
+ pixel_coords = get_pixel_3d_coordinates(
+ self.correlation_target.fm_streams[0], coords, check_bbox=True
+ )
+ if pixel_coords is None:
+ wx.MessageBox("Target must be inside the FM volume to refine it",
+ "Error", wx.OK | wx.ICON_ERROR)
+ return
+
+ target_x, target_y = round(pixel_coords[0]), round(pixel_coords[1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/odemis/gui/cont/multi_point_correlation.py` around lines 823 - 826, The
current call to get_pixel_3d_coordinates allows seed points outside the FM
volume; before building the ROI you must reject out-of-bounds targets. After
obtaining coords from
self._tab_data_model.main.currentTarget.value.coordinates.value and/or when
calling get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords),
validate the resulting pixel X/Y against the FM stream's pixel bounds (or call
get_pixel_3d_coordinates with check_bbox=True if available) and if X or Y is
negative or >= the stream's width/height, abort/return early (or mark the target
invalid) instead of proceeding to build the ROI; reference the variables
self._tab_data_model.main.currentTarget, get_pixel_3d_coordinates, and
self.correlation_target.fm_streams[0] when locating where to add this check.
2f6a326 to
3ddfac6
Compare
tmoerkerken
force-pushed
the
use-com-for-fm-refinement
branch
from
3ddfac6 to
dd89fe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/odemis/gui/cont/multi_point_correlation.py (2)
851-853:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard pixel-size dimensionality before reading
pxs[2].
MD_PIXEL_SIZEdefaults to 2 values, but the code always indexespxs[2]. This can raiseIndexErroron 2D metadata.Proposed fix
pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) - com_pixel_z_iso = com_pixel_z * pxs[2] / pxs[0] + pxs_z = pxs[2] if len(pxs) >= 3 else pxs[0] + com_pixel_z_iso = com_pixel_z * pxs_z / pxs[0]🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 851 - 853, Guard against 2D pixel-size metadata before indexing pxs[2]: retrieve pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)), then compute a z-scale fallback when pxs has only two elements (e.g., z_scale = pxs[2] if len(pxs) > 2 else pxs[0] or 1e-6), and use that in the existing computation of com_pixel_z_iso (com_pixel_z_iso = com_pixel_z * z_scale / pxs[0]); update the code around the pxs, com_pixel_z and com_pixel_z_iso usage in multi_point_correlation.py accordingly.
828-831:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-bounds targets before ROI construction.
get_pixel_3d_coordinates(..., check_bbox=False)allows invalid seed points, andpixel_coordsis used immediately without a bounds/None check. This can drive ROI extraction from unrelated regions or fail unpredictably.Proposed fix
- pixel_coords = get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords) + pixel_coords = get_pixel_3d_coordinates( + self.correlation_target.fm_streams[0], coords, check_bbox=True + ) + if pixel_coords is None: + wx.MessageBox( + "Target must be inside the FM volume to refine it", + "Error", + wx.OK | wx.ICON_ERROR, + ) + return - target_x, target_y = int(pixel_coords[0]), int(pixel_coords[1]) + target_x, target_y = round(pixel_coords[0]), round(pixel_coords[1])🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 828 - 831, The call to get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords) can return invalid/out-of-bounds data and pixel_coords is used immediately; validate pixel_coords (check for None and that pixel_coords[0], pixel_coords[1] lie inside the image bounds of self.correlation_target.fm_streams[0] or the ROI source) before converting to ints and before any ROI construction (reject or early-return the seed if invalid). Use the stream/frame dimensions from the same fm_streams[0] to compute valid x/y ranges and handle the invalid case by skipping the seed or raising a controlled error so ROI extraction code never operates on out-of-range coordinates.
🤖 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/odemis/gui/cont/multi_point_correlation.py`:
- Line 811: The method _on_xyz_targeting has an untyped parameter evt; add an
explicit type annotation (e.g. evt: wx.Event if using wx, or evt: Any if event
type is generic) and update the signature to def _on_xyz_targeting(self, evt:
<EventType>) -> None:, and add the matching import (from wx import Event or from
typing import Any) at the top of the module so the code conforms to the
repository's typing rules.
- Around line 833-843: The code currently stacks all entries in
streams_projections into raw_multi which can include non-FM projections; before
building raw_multi, filter streams_projections to only include FM/fluorescence
projection entries (e.g. check a property like stream.stream_type == 'FM' or
stream.stream.get('modality') == 'FM') and then build raw_multi from that
filtered list; update the raw_multi comprehension and add a guard (raise/log) if
the filtered list is empty to avoid invalid shapes before calling
get_brightest_channel, compute_local_center_of_mass, and any downstream logic.
---
Duplicate comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 851-853: Guard against 2D pixel-size metadata before indexing
pxs[2]: retrieve pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)), then compute a
z-scale fallback when pxs has only two elements (e.g., z_scale = pxs[2] if
len(pxs) > 2 else pxs[0] or 1e-6), and use that in the existing computation of
com_pixel_z_iso (com_pixel_z_iso = com_pixel_z * z_scale / pxs[0]); update the
code around the pxs, com_pixel_z and com_pixel_z_iso usage in
multi_point_correlation.py accordingly.
- Around line 828-831: The call to
get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords) can
return invalid/out-of-bounds data and pixel_coords is used immediately; validate
pixel_coords (check for None and that pixel_coords[0], pixel_coords[1] lie
inside the image bounds of self.correlation_target.fm_streams[0] or the ROI
source) before converting to ints and before any ROI construction (reject or
early-return the seed if invalid). Use the stream/frame dimensions from the same
fm_streams[0] to compute valid x/y ranges and handle the invalid case by
skipping the seed or raising a controlled error so ROI extraction code never
operates on out-of-range coordinates.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9109d86e-cbe5-419d-9707-5b10b86725f8
📒 Files selected for processing (6)
src/odemis/gui/cont/acquisition/cryo_z_localization.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/util/img.pysrc/odemis/util/test/img_test.py
✅ Files skipped from review due to trivial changes (1)
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
🚧 Files skipped from review as they are similar to previous changes (4)
- src/odemis/gui/cont/acquisition/cryo_z_localization.py
- src/odemis/gui/main_xrc.py
- src/odemis/util/test/img_test.py
- src/odemis/util/img.py
| self._need_reprocessing() | ||
|
|
||
| def _on_z_targeting(self, evt) -> None: | ||
| def _on_xyz_targeting(self, evt) -> None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a type hint for evt in _on_xyz_targeting.
The new method parameter is untyped, which violates the repository Python typing rule.
- def _on_xyz_targeting(self, evt) -> None:
+ def _on_xyz_targeting(self, evt: Optional[wx.CommandEvent]) -> None:As per coding guidelines "Always use type hints for function parameters and return types in Python code".
🤖 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/odemis/gui/cont/multi_point_correlation.py` at line 811, The method
_on_xyz_targeting has an untyped parameter evt; add an explicit type annotation
(e.g. evt: wx.Event if using wx, or evt: Any if event type is generic) and
update the signature to def _on_xyz_targeting(self, evt: <EventType>) -> None:,
and add the matching import (from wx import Event or from typing import Any) at
the top of the module so the code conforms to the repository's typing rules.
Member
There was a problem hiding this comment.
As this function is also called explicitly from other function, without any event, you could change the signature to be:
Suggested change
| def _on_xyz_targeting(self, evt) -> None: | |
| def _on_xyz_targeting(self, evt: Optional[wx.Event] = None) -> None: |
This way, you can call the function without argument.
There was a problem hiding this comment.
@pieleric, that's a great improvement — making evt default to None is cleaner because callers don't need to pass an explicit None. Your suggested signature is the right approach:
def _on_xyz_targeting(self, evt: Optional[wx.Event] = None) -> None:As a minor follow-up, the existing call site in _on_current_target_changes could then also be simplified from self._on_xyz_targeting(None) to just self._on_xyz_targeting(), but that's entirely optional.
🧠 Learnings used
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
dd89fe8 to
2f6ca60
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/odemis/gui/cont/multi_point_correlation.py (3)
818-833:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict
raw_multito FM stacks only.
stream_tree.flat.valuealso contains the FIB projection added to this view. Stacking every visible stream here can mix 2D FIB data with 3D FM volumes, which turnsraw_multiinto an invalid/object array before COM runs.Proposed fix
- streams_projections = self._tab_data_model.views.value[0].stream_tree.flat.value + streams_projections = [ + projection + for projection in self._tab_data_model.views.value[0].stream_tree.flat.value + if isinstance(projection.stream, StaticFluoStream) + ] if not streams_projections: wx.MessageBox("FM streams are not available for refining targets", "Error", wx.OK | wx.ICON_ERROR) return🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 818 - 833, The current construction of raw_multi from streams_projections can include 2D FIB projections and produce an object/invalid array; update the selection to only include FM (3D) stacks before building raw_multi — e.g. filter streams_projections for entries where the underlying stream has 3D raw data (check attributes on each item like d.stream.raw and use getattr(d.stream.raw, "ndim", 0) >= 3 or len(d.stream.raw) > 1) and then build raw_multi from that filtered list so raw_multi remains a numeric 3D stack (change the comprehension that assigns raw_multi to first filter streams_projections accordingly).
827-843:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-bounds targets before building the ROI.
get_pixel_3d_coordinates(..., check_bbox=False)lets negative or oversized X/Y through. That can produce an empty crop here, and the new COM path then fails on a zero-sized array instead of giving the user a recoverable error.Proposed fix
- pixel_coords = get_pixel_3d_coordinates(self.correlation_target.fm_streams[0], coords) + pixel_coords = get_pixel_3d_coordinates( + self.correlation_target.fm_streams[0], coords, check_bbox=True + ) + if pixel_coords is None: + wx.MessageBox("Target must be inside the FM volume to refine it", + "Error", wx.OK | wx.ICON_ERROR) + return - target_x, target_y = int(pixel_coords[0]), int(pixel_coords[1]) + target_x, target_y = round(pixel_coords[0]), round(pixel_coords[1])🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 827 - 843, The code allows out-of-bounds pixel coordinates from get_pixel_3d_coordinates to produce an empty crop and later fail; before building the ROI (after computing target_x/target_y and shape_x/shape_y), validate that 0 <= target_x < shape_x and 0 <= target_y < shape_y (using shape_x/shape_y from raw_multi) and handle invalid targets by returning an error or raising a clear exception (or prompting the caller) instead of proceeding to create multi_crop/compute COM; this check should be applied just before computing y_start/y_end/x_start/x_end and references variables get_pixel_3d_coordinates, target_x, target_y, raw_multi, multi_crop, COM_ROI_PADDING and streams_projections.
854-863:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle 2D pixel-size metadata before reading
pxs[2].The fallback passed to
md.get()only has two elements, but the next line unconditionally readspxs[2]. Any FM stack that only stores XY spacing will crash during refinement instead of defaulting to the existing iso-voxel assumption.Proposed fix
best_stream = streams_projections[best_c].stream best_channel_raw = best_stream.raw[0] md = best_stream._find_metadata(best_channel_raw.metadata) pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) - com_y = com_z * pxs[2] / pxs[0] + pxs_z = pxs[2] if len(pxs) >= 3 else pxs[0] + com_z_iso = com_z * pxs_z / pxs[0] @@ self.correlation_target.fm_streams[0], - (com_x_crop, com_y_crop, com_y) + (com_x_crop, com_y_crop, com_z_iso) )🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 854 - 863, The code reads pxs = md.get(model.MD_PIXEL_SIZE, (1e-6, 1e-6)) then immediately indexes pxs[2], which will crash for 2D-only metadata; fix by ensuring pxs has three elements before using pxs[2]—either request a 3-tuple fallback (e.g. (1e-6,1e-6,1e-6)) or pad/expand the returned pxs to length 3 (set z spacing = x spacing when missing) before computing com_y = com_z * pxs[2] / pxs[0]; update the block around md, pxs, com_y and ensure the value passed into get_physical_3d_coordinates (via self.correlation_target.fm_streams[0]) uses the corrected 3D spacing.
🧹 Nitpick comments (1)
src/odemis/util/test/img_test.py (1)
2345-2368: ⚡ Quick winExercise a non-zero ROI offset in this test.
y_startandx_startboth resolve to0, so this never verifies the crop-to-global re-offset that_on_xyz_targeting()depends on. An offset bug would still pass here.Proposed fix
- target_y = 5 + target_y = 10 target_x = 10 pad_y = 5 - pad_x = 10 + pad_x = 5 @@ - com = img.compute_center_of_mass(sub_image, baseline_percentile=95.0) + com = img.compute_center_of_mass(sub_image[roi], baseline_percentile=95.0) com_y_crop = com[1] + roi[1].start com_x_crop = com[2] + roi[2].start🤖 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/odemis/util/test/img_test.py` around lines 2345 - 2368, The test never exercises a non-zero ROI offset because y_start and x_start end up 0; change the target and/or pad values so roi[1].start and roi[2].start are > 0 (for example increase target_y and target_x to values > pad_y/pad_x or reduce pad values) so compute_center_of_mass(sub_image, ...) returns com and the subsequent com_y_crop = com[1] + roi[1].start and com_x_crop = com[2] + roi[2].start actually test the re-offset logic used by _on_xyz_targeting(); update the assertions to use the new expected ROI bounds (based on the recomputed y_start/y_end and x_start/x_end) and keep calls/identifiers compute_center_of_mass, roi, com_y_crop, com_x_crop unchanged.
🤖 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/odemis/util/img.py`:
- Line 1473: The file ends with a bare "return com" line and is missing a
trailing newline causing lint W292; fix by adding a single newline character at
the end of the file (after the final "return com" statement) so the file ends
with a newline.
- Around line 1458-1473: compute_center_of_mass can call
scipy.ndimage.center_of_mass on an all-zero weights array after percentile
baselining, producing invalid coordinates that downstream _on_xyz_targeting
writes back; modify compute_center_of_mass to check if the computed
image_weights has any positive values (e.g., sum or any > 0) before calling
scipy.ndimage.center_of_mass and if none are present raise a clear exception
(e.g., ValueError with context including baseline_percentile) so callers like
_on_xyz_targeting can handle the empty/all-background ROI instead of writing
corrupt coordinates.
---
Duplicate comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 818-833: The current construction of raw_multi from
streams_projections can include 2D FIB projections and produce an object/invalid
array; update the selection to only include FM (3D) stacks before building
raw_multi — e.g. filter streams_projections for entries where the underlying
stream has 3D raw data (check attributes on each item like d.stream.raw and use
getattr(d.stream.raw, "ndim", 0) >= 3 or len(d.stream.raw) > 1) and then build
raw_multi from that filtered list so raw_multi remains a numeric 3D stack
(change the comprehension that assigns raw_multi to first filter
streams_projections accordingly).
- Around line 827-843: The code allows out-of-bounds pixel coordinates from
get_pixel_3d_coordinates to produce an empty crop and later fail; before
building the ROI (after computing target_x/target_y and shape_x/shape_y),
validate that 0 <= target_x < shape_x and 0 <= target_y < shape_y (using
shape_x/shape_y from raw_multi) and handle invalid targets by returning an error
or raising a clear exception (or prompting the caller) instead of proceeding to
create multi_crop/compute COM; this check should be applied just before
computing y_start/y_end/x_start/x_end and references variables
get_pixel_3d_coordinates, target_x, target_y, raw_multi, multi_crop,
COM_ROI_PADDING and streams_projections.
- Around line 854-863: The code reads pxs = md.get(model.MD_PIXEL_SIZE, (1e-6,
1e-6)) then immediately indexes pxs[2], which will crash for 2D-only metadata;
fix by ensuring pxs has three elements before using pxs[2]—either request a
3-tuple fallback (e.g. (1e-6,1e-6,1e-6)) or pad/expand the returned pxs to
length 3 (set z spacing = x spacing when missing) before computing com_y = com_z
* pxs[2] / pxs[0]; update the block around md, pxs, com_y and ensure the value
passed into get_physical_3d_coordinates (via
self.correlation_target.fm_streams[0]) uses the corrected 3D spacing.
---
Nitpick comments:
In `@src/odemis/util/test/img_test.py`:
- Around line 2345-2368: The test never exercises a non-zero ROI offset because
y_start and x_start end up 0; change the target and/or pad values so
roi[1].start and roi[2].start are > 0 (for example increase target_y and
target_x to values > pad_y/pad_x or reduce pad values) so
compute_center_of_mass(sub_image, ...) returns com and the subsequent com_y_crop
= com[1] + roi[1].start and com_x_crop = com[2] + roi[2].start actually test the
re-offset logic used by _on_xyz_targeting(); update the assertions to use the
new expected ROI bounds (based on the recomputed y_start/y_end and
x_start/x_end) and keep calls/identifiers compute_center_of_mass, roi,
com_y_crop, com_x_crop unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 303b6240-c6ef-437b-8330-180d2e6d3683
📒 Files selected for processing (6)
src/odemis/gui/cont/acquisition/cryo_z_localization.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/util/img.pysrc/odemis/util/test/img_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/gui/cont/acquisition/cryo_z_localization.py
- src/odemis/gui/main_xrc.py
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
Comment on lines
+1458
to
+1473
| def compute_center_of_mass(image: numpy.ndarray, | ||
| baseline_percentile: float = 95.0, roi_slice: tuple = None) -> Tuple[float, float, float]: | ||
| """ | ||
| Computes center of mass with baselining. | ||
| Uses the brightest pixels (above the baseline percentile) as weights for COM calculation, | ||
| effectively filtering background noise. | ||
|
|
||
| :param image: Single-channel 3D sub-image as (Z, Y, X) array | ||
| :param baseline_percentile: Percentile threshold for background separation (0-100). Values lower than the baseline | ||
| will be discarded. | ||
| :returns: Tuple of (global_z, global_y, global_x) in pixel coordinates maintaining input order. | ||
| """ | ||
| baseline = numpy.percentile(image, baseline_percentile) | ||
| image_weights = numpy.where(image > baseline, image - baseline, 0) | ||
| com = scipy.ndimage.center_of_mass(numpy.asarray(image_weights)) | ||
| return com No newline at end of file |
There was a problem hiding this comment.
Guard the empty/all-background case before calling COM.
If percentile filtering leaves no positive weights, scipy.ndimage.center_of_mass() returns invalid coordinates. _on_xyz_targeting() then writes those straight back into the active target, so a flat or empty ROI can corrupt the point instead of failing cleanly.
Proposed fix
def compute_center_of_mass(image: numpy.ndarray,
baseline_percentile: float = 95.0, roi_slice: tuple = None) -> Tuple[float, float, float]:
@@
+ if image.size == 0:
+ raise ValueError("Cannot compute center of mass of an empty image")
baseline = numpy.percentile(image, baseline_percentile)
image_weights = numpy.where(image > baseline, image - baseline, 0)
+ if not numpy.any(image_weights):
+ raise ValueError("Center of mass is undefined for an empty foreground mask")
com = scipy.ndimage.center_of_mass(numpy.asarray(image_weights))
return com🧰 Tools
🪛 GitHub Actions: Linting / 0_lint.txt
[error] 1473-1473: flake8 failed with W292: no newline at end of file. Process completed with exit code 1.
🪛 GitHub Actions: Linting / lint
[error] 1473-1473: flake8 failed with selected rules W291,W292,W293,W391,E999. W292 no newline at end of file.
🪛 Ruff (0.15.12)
[warning] 1459-1459: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 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/odemis/util/img.py` around lines 1458 - 1473, compute_center_of_mass can
call scipy.ndimage.center_of_mass on an all-zero weights array after percentile
baselining, producing invalid coordinates that downstream _on_xyz_targeting
writes back; modify compute_center_of_mass to check if the computed
image_weights has any positive values (e.g., sum or any > 0) before calling
scipy.ndimage.center_of_mass and if none are present raise a clear exception
(e.g., ValueError with context including baseline_percentile) so callers like
_on_xyz_targeting can handle the empty/all-background ROI instead of writing
corrupt coordinates.
d744044 to
266084b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/odemis/util/img.py (2)
1473-1473:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing trailing newline.
This still fails the current lint job with
W292.🤖 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/odemis/util/img.py` at line 1473, Add the missing trailing newline at the end of the file by ensuring the file terminates with a '\n' character after the final statement (the line containing "return com"); this will resolve the W292 lint error.
1470-1472:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the empty/all-background case before calling COM.
If percentile baselining leaves no positive weights,
scipy.ndimage.center_of_mass()can return invalid coordinates, and the new refinement path will write those back into the target instead of failing cleanly.Proposed fix
+ if image.size == 0: + raise ValueError("Cannot compute center of mass of an empty image") baseline = numpy.percentile(image, baseline_percentile) image_weights = numpy.where(image > baseline, image - baseline, 0) + if not numpy.any(image_weights): + raise ValueError( + f"Center of mass is undefined for baseline_percentile={baseline_percentile}" + ) com = scipy.ndimage.center_of_mass(numpy.asarray(image_weights))🤖 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/odemis/util/img.py` around lines 1470 - 1472, The percentile baselining code computes image_weights and immediately calls scipy.ndimage.center_of_mass (com) which can return invalid coordinates when image_weights contains no positive values; before calling center_of_mass (or before using its result), check whether numpy.any(image_weights > 0) (or equivalently image_weights.sum() > 0) and handle the empty/all-background case by skipping refinement or returning a safe default/error instead of using the invalid com; update the logic around baseline, image_weights and com to early-return or no-op when there are no positive weights to prevent writing invalid coordinates back into the target.src/odemis/gui/cont/multi_point_correlation.py (2)
838-845:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-bounds targets before building the ROI.
get_pixel_3d_coordinates()is still called without bbox checking here, so a target edited outside the FM volume can produce an empty or unrelated crop and refine against the wrong data.Proposed fix
- pixel_coords = get_pixel_3d_coordinates(streams[0], coords) + pixel_coords = get_pixel_3d_coordinates(streams[0], coords, check_bbox=True) + if pixel_coords is None: + wx.MessageBox( + "Target must be inside the FM volume to refine it", + "Error", + wx.OK | wx.ICON_ERROR, + ) + return # We are going to refine around the clicked position - target_x, target_y = int(pixel_coords[0]), int(pixel_coords[1]) + target_x, target_y = round(pixel_coords[0]), round(pixel_coords[1])🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 838 - 845, The code calls get_pixel_3d_coordinates and then builds an ROI without rejecting targets outside the FM volume; update the block to first validate the target against the stream bounds (either check the world-space bbox on self._tab_data_model.main.currentTarget.value.coordinates.value against streams[0] volume/bbox or call get_pixel_3d_coordinates and then immediately validate pixel_coords against the image extents) and if out-of-bounds, abort/return early (or show an error) instead of proceeding to compute target_x/target_y, constructing raw_multi, or building the ROI; reference get_pixel_3d_coordinates, self._tab_data_model.main.currentTarget, streams[0], pixel_coords, target_x/target_y and raw_multi when locating where to add the checks.
702-708:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestrict refinement candidates to fluorescence streams.
The
MD_OUT_WL != BAND_PASS_THROUGHpredicate still lets the FIB/SEM reference stream through when that metadata is absent. That can breakstream.max_projectionhere and later build a mixed-dimensionraw_multiduring refinement.Proposed fix
- visible_streams = [ - s.stream for s in self._tab_data_model.views.value[0].stream_tree.flat.value - if s.stream.getRawMetadata()[0].get(model.MD_OUT_WL) != model.BAND_PASS_THROUGH - ] + visible_streams = [ + s.stream for s in self._tab_data_model.views.value[0].stream_tree.flat.value + if isinstance(s.stream, StaticFluoStream) + and s.stream.getRawMetadata()[0].get(model.MD_OUT_WL) != model.BAND_PASS_THROUGH + ] @@ - streams = [ - s.stream for s in self._tab_data_model.views.value[0].stream_tree.flat.value - if s.stream.getRawMetadata()[0].get(model.MD_OUT_WL) != model.BAND_PASS_THROUGH - ] + streams = [ + s.stream for s in self._tab_data_model.views.value[0].stream_tree.flat.value + if isinstance(s.stream, StaticFluoStream) + and s.stream.getRawMetadata()[0].get(model.MD_OUT_WL) != model.BAND_PASS_THROUGH + ]Also applies to: 826-830
🤖 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/odemis/gui/cont/multi_point_correlation.py` around lines 702 - 708, The current visible_streams filter allows streams lacking MD_OUT_WL (e.g., FIB/SEM) through, causing stream.max_projection access and mixed-dimension raw_multi errors; update the filter in multi_point_correlation.py (around the visible_streams/mip_enabled logic) to only include fluorescence streams by checking the metadata key exists and equals a fluorescence band (i.e., ensure stream.getRawMetadata()[0].get(model.MD_OUT_WL) is present and not BAND_PASS_THROUGH) and/or that the stream exposes max_projection (e.g., hasattr(stream, "max_projection") or stream.max_projection is not None) before computing mip_enabled and before using those streams in refinement to prevent non-fluorescence streams from being included.
🧹 Nitpick comments (1)
src/odemis/util/img.py (1)
1458-1459: ⚡ Quick winUse an explicit optional type for
roi_slice, or drop it until it is used.
roi_slice: tuple = Noneis the implicit-optional form Ruff is flagging here, and it does not meet the repo's Python 3.10 typing rule.Proposed fix
-def compute_center_of_mass(image: numpy.ndarray, - baseline_percentile: float = 95.0, roi_slice: tuple = None) -> Tuple[float, float, float]: +def compute_center_of_mass( + image: numpy.ndarray, + baseline_percentile: float = 95.0, + roi_slice: tuple[slice, ...] | None = None, +) -> Tuple[float, float, float]:As per coding guidelines "Always use type hints for function parameters and return types in Python code" and "Code should be valid for Python 3.10 and above".
🤖 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/odemis/util/img.py` around lines 1458 - 1459, Change the implicit-optional parameter roi_slice in compute_center_of_mass to use an explicit Optional type (e.g. roi_slice: Optional[Tuple[slice, ...]] = None) and add the required import from typing (Optional) at the top of the file; ensure the signature remains compute_center_of_mass(image: numpy.ndarray, baseline_percentile: float = 95.0, roi_slice: Optional[Tuple[slice, ...]] = None) -> Tuple[float, float, float] so type checkers accept the Python 3.10 typing rule.
🤖 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.
Duplicate comments:
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 838-845: The code calls get_pixel_3d_coordinates and then builds
an ROI without rejecting targets outside the FM volume; update the block to
first validate the target against the stream bounds (either check the
world-space bbox on
self._tab_data_model.main.currentTarget.value.coordinates.value against
streams[0] volume/bbox or call get_pixel_3d_coordinates and then immediately
validate pixel_coords against the image extents) and if out-of-bounds,
abort/return early (or show an error) instead of proceeding to compute
target_x/target_y, constructing raw_multi, or building the ROI; reference
get_pixel_3d_coordinates, self._tab_data_model.main.currentTarget, streams[0],
pixel_coords, target_x/target_y and raw_multi when locating where to add the
checks.
- Around line 702-708: The current visible_streams filter allows streams lacking
MD_OUT_WL (e.g., FIB/SEM) through, causing stream.max_projection access and
mixed-dimension raw_multi errors; update the filter in
multi_point_correlation.py (around the visible_streams/mip_enabled logic) to
only include fluorescence streams by checking the metadata key exists and equals
a fluorescence band (i.e., ensure
stream.getRawMetadata()[0].get(model.MD_OUT_WL) is present and not
BAND_PASS_THROUGH) and/or that the stream exposes max_projection (e.g.,
hasattr(stream, "max_projection") or stream.max_projection is not None) before
computing mip_enabled and before using those streams in refinement to prevent
non-fluorescence streams from being included.
In `@src/odemis/util/img.py`:
- Line 1473: Add the missing trailing newline at the end of the file by ensuring
the file terminates with a '\n' character after the final statement (the line
containing "return com"); this will resolve the W292 lint error.
- Around line 1470-1472: The percentile baselining code computes image_weights
and immediately calls scipy.ndimage.center_of_mass (com) which can return
invalid coordinates when image_weights contains no positive values; before
calling center_of_mass (or before using its result), check whether
numpy.any(image_weights > 0) (or equivalently image_weights.sum() > 0) and
handle the empty/all-background case by skipping refinement or returning a safe
default/error instead of using the invalid com; update the logic around
baseline, image_weights and com to early-return or no-op when there are no
positive weights to prevent writing invalid coordinates back into the target.
---
Nitpick comments:
In `@src/odemis/util/img.py`:
- Around line 1458-1459: Change the implicit-optional parameter roi_slice in
compute_center_of_mass to use an explicit Optional type (e.g. roi_slice:
Optional[Tuple[slice, ...]] = None) and add the required import from typing
(Optional) at the top of the file; ensure the signature remains
compute_center_of_mass(image: numpy.ndarray, baseline_percentile: float = 95.0,
roi_slice: Optional[Tuple[slice, ...]] = None) -> Tuple[float, float, float] so
type checkers accept the Python 3.10 typing rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3b7a3b1-7485-4bc1-8853-7d408700deb7
📒 Files selected for processing (6)
src/odemis/gui/cont/acquisition/cryo_z_localization.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/util/img.pysrc/odemis/util/test/img_test.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/odemis/util/test/img_test.py
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
- src/odemis/gui/main_xrc.py
- src/odemis/gui/cont/acquisition/cryo_z_localization.py
tmoerkerken
force-pushed
the
use-com-for-fm-refinement
branch
2 times, most recently
from
56c8da0 to
7b45962
Compare
tmoerkerken
changed the title
tmoerkerken
force-pushed
the
use-com-for-fm-refinement
branch
from
7b45962 to
8117bd4
Compare
pieleric
requested changes
May 12, 2026
tmoerkerken
force-pushed
the
use-com-for-fm-refinement
branch
from
8117bd4 to
d5ba031
Compare
K4rishma
reviewed
May 22, 2026
| channel_maxes = numpy.max(sub_image_multi, axis=(1, 2, 3)) | ||
| return int(numpy.argmax(channel_maxes)) | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
extra blank line to be removed
K4rishma
reviewed
May 22, 2026
| baseline = numpy.percentile(image, baseline_ratio * 100) | ||
| image_weights = numpy.where(image > baseline, image - baseline, 0) | ||
| com = scipy.ndimage.center_of_mass(numpy.asarray(image_weights)) | ||
| return com |
Contributor
There was a problem hiding this comment.
empty line to be added at the end of the file
K4rishma
reviewed
May 22, 2026
| baseline_ratio: float = 0.95, roi_slice: tuple = None) -> Tuple[float, float, float]: | ||
| """ | ||
| Computes center of mass with baselining. | ||
| Uses the brightest pixels (above the baseline percentile) as weights for COM calculation, |
Contributor
There was a problem hiding this comment.
COM I would write the full form in the function description
K4rishma
reviewed
May 22, 2026
| """ | ||
| return [px // (2 ** z) for px in rect] | ||
|
|
||
|
|
K4rishma
reviewed
May 22, 2026
|
|
||
| def get_brightest_channel(sub_image_multi: numpy.ndarray) -> int: | ||
| """ | ||
| Finds the index of the channel with the highest maximum intensity. |
Contributor
There was a problem hiding this comment.
highest maximum -> maximum
K4rishma
reviewed
May 22, 2026
| Useful for multi-channel imaging to select the brightest channel for analysis. | ||
|
|
||
| :param sub_image_multi: Multi-channel sub-image as (C, Z, Y, X) array | ||
| :returns: Index of the channel with the highest intensity |
Contributor
There was a problem hiding this comment.
highest intensity --> maximum intensity
K4rishma
approved these changes
May 22, 2026
pieleric
requested changes
May 22, 2026
| # VAs for currently selected targets | ||
| self.targets = model.ListVA() | ||
| self.currentTarget = model.VigilantAttribute(None) | ||
| self.needs_target_refinement = model.BooleanVA(False) |
Member
There was a problem hiding this comment.
The MainGUIData is supposed to contain the state global to the entire GUI. I don't think that is useful here. You could move it "down" to the CryoGUIData, which represent the state just for a single tab. However, I think that what would be even more logical is to store the info directly on the Target. I think that what you'd want to indicate is "z is not defined yet (and needs to be computed based on data)".
So I'd suggest to extend Target, and either:
- support passing
Nonein z, to indicate it's unknown and needs recomputation. That was my first feeling, but looking at the code, there are lots of expectation that z is a float, which are going to be annoying to lift. - add an extra argument/attribute
z_needs_refinement(bool, default = False)
Then, you can adjust add_new_target() to pass the new argument in the right cases, and _on_current_target_changes can directly look on its target argument.
| physical_coords = get_physical_3d_coordinates( | ||
| visible_streams[0], (0, 0, visible_streams[0].zIndex.value)) | ||
| # Update the model with the refined 3D coordinates | ||
| target_coords = self._tab_data_model.main.currentTarget.value.coordinates.value |
Member
There was a problem hiding this comment.
The function receives self._tab_data_model.main.currentTarget.value as argument, so you can simply write:
Suggested change
| target_coords = self._tab_data_model.main.currentTarget.value.coordinates.value | |
| target_coords = target.coordinates.value |
| self._need_reprocessing() | ||
|
|
||
| def _on_z_targeting(self, evt) -> None: | ||
| def _on_xyz_targeting(self, evt) -> None: |
Member
There was a problem hiding this comment.
As this function is also called explicitly from other function, without any event, you could change the signature to be:
Suggested change
| def _on_xyz_targeting(self, evt) -> None: | |
| def _on_xyz_targeting(self, evt: Optional[wx.Event] = None) -> None: |
This way, you can call the function without argument.
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
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.
This PR introduces a variety of improvements to the 3D correlation workflow:
→ Now using improved drawing calls that automatically update the viewports when fiducials change
→ Auto refinement overwrote manual edits, which is undesirable
Comparison
Bad input
We see that the new algorithm is very good at recovering from bad user input. This makes placement of fiducials really simple. New method: Center of Mass (new), Gauss fit 1d (old).
Other examples
See JIRA task for more examples
Out of scope
I encountered various peculiar UX related things (listed in the JIRA ticket) that I'm not touching right now. We could improve upon them later.