[MSD-616][feat] FIB-view FM correlation workflow#3498
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds posture-aware behavior for a new "FIB view FM" state across acquisition, feature, and correlation controllers. Sequence Diagram(s)sequenceDiagram
participant User
participant LocalizationTab
participant CorrelationController
participant PostureManager
User->>LocalizationTab: Stage moves
LocalizationTab->>PostureManager: query current posture
PostureManager-->>LocalizationTab: FIB_VIEW_FM / FM_IMAGING
LocalizationTab->>LocalizationTab: update posture indicator + overview button
User->>CorrelationController: initiate correlation
CorrelationController->>PostureManager: check at_fib_view_fm
alt at_fib_view_fm
CorrelationController->>CorrelationController: _do_2d_correlation() (SimilarityTransform)
else
CorrelationController->>CorrelationController: _do_3d_correlation()
end
CorrelationController-->>User: projected POI/fiducials + rms_error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/driver/static.py (1)
46-58: 📐 Maintainability & Code Quality | 🟡 MinorAdd type hints to
OpticalLens.__init__parameters and return type, and convert docstring to reStructuredText:paramstyle without inline type information.The constructor is missing type hints for all parameters and has no return type annotation. The docstring uses inline type formatting (e.g.,
name (string):,mag (float > 0):) instead of the required reStructuredText:paramformat without type info.🤖 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/driver/static.py` around lines 46 - 58, The OpticalLens.__init__ method is missing type annotations for all parameters and lacks a return type hint. Convert the docstring from inline type formatting (e.g., "name (string):", "mag (float > 0):") to reStructuredText :param style without embedding type information in the parameter descriptions. Add appropriate Python type hints to all parameters including name, role, mag, mag_choices, na, ri, wd, pole_pos, mirror_pos_top, mirror_pos_bottom, x_max, hole_diam, focus_dist, parabola_f, rotation, configurations, and kwargs, then add a return type annotation of None to the __init__ method signature.Source: Coding guidelines
🧹 Nitpick comments (3)
src/odemis/gui/cont/multi_point_correlation.py (1)
544-545: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type hint on the new helper.
_do_2d_correlationshould declare-> Noneto match the repository typing rule.As per coding guidelines, `**/*.py`: Always use type hints for function parameters and return types in Python code.💡 Proposed fix
- def _do_2d_correlation(self): + def _do_2d_correlation(self) -> None: """Run the 2d correlation between the FIB and FM images."""🤖 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 544 - 545, The method _do_2d_correlation is missing an explicit return type hint. Add `-> None` to the method signature after the closing parenthesis of the parameter list to comply with the repository's typing requirements, since this method does not return any value.Source: Coding guidelines
src/odemis/gui/cont/tabs/localization_tab.py (1)
498-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding else clause to clear stale posture indicator.
The posture indicator is only updated for
FIB_VIEW_FMandFM_IMAGING. If the stage moves to another posture, the label and bitmap will retain their previous values. While the tab should be disabled at other postures, an else clause would make the logic more explicit and prevent potentially confusing UI state.♻️ Proposed enhancement
self._acquisition_controller._update_overview_acquisition_button() # Update the current posture read-only indicator if needed if self.main_data.posture_manager.at_fib_view_fm_posture(pos): self.panel.lbl_current_posture.SetLabel(POSITION_NAMES[FIB_VIEW_FM]) self.panel.bmp_current_posture.SetBitmap(self.bmp_fib_view_fm) elif self.main_data.posture_manager.at_fm_imaging_posture(pos): self.panel.lbl_current_posture.SetLabel(POSITION_NAMES[FM_IMAGING]) self.panel.bmp_current_posture.SetBitmap(self.bmp_fm_imaging) +else: + # Clear indicator when at other postures (tab should be disabled) + self.panel.lbl_current_posture.SetLabel("") + self.panel.bmp_current_posture.SetBitmap(wx.NullBitmap)🤖 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/tabs/localization_tab.py` around lines 498 - 505, The posture indicator update logic in the if/elif block only handles FIB_VIEW_FM and FM_IMAGING postures but lacks an else clause to clear stale UI state when the stage moves to other postures. Add an else clause after the elif statement that clears both self.panel.lbl_current_posture and self.panel.bmp_current_posture by setting the label to an empty string and the bitmap to an empty or default bitmap value to ensure the UI does not retain outdated posture information.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
769-785: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the posture check result.
The method calls
pm.at_fib_view_fm_posture(pm.stage.position.value)twice (lines 776 and 779). Store the result in a variable to avoid redundant computation.♻️ Proposed refactor
`@call_in_wx_main` def _update_overview_acquisition_button(self) -> None: """ Update the overview acquisition button enabled state to reflect the current UI state, based on if an acquisition is already taking place or the current stage posture. """ pm = self._tab_data.main.posture_manager - enabled = not self._tab_data.main.is_acquiring.value and not pm.at_fib_view_fm_posture(pm.stage.position.value) + at_fib_view_fm = pm.at_fib_view_fm_posture(pm.stage.position.value) + enabled = not self._tab_data.main.is_acquiring.value and not at_fib_view_fm self._panel.btn_acquire_overview.Enable(enabled) # Only show a tooltip when at FM Milling. The other option, while acquiring, is quite obvious. - if pm.at_fib_view_fm_posture(pm.stage.position.value): + if at_fib_view_fm: self._panel.btn_acquire_overview.SetToolTip( f"Overview acquisition is not available when at {POSITION_NAMES[FIB_VIEW_FM]} posture" ) else: self._panel.btn_acquire_overview.SetToolTip("")🤖 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/acquisition/cryo_acq.py` around lines 769 - 785, The method _update_overview_acquisition_button is calling pm.at_fib_view_fm_posture(pm.stage.position.value) twice, which is inefficient. Store the result of this method call in a local variable before using it in both the enabled calculation and the tooltip condition, so the posture check is performed only once instead of twice.
🤖 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/acq/move.py`:
- Around line 382-383: The condition comparing lens.workingDistance.value
against MIN_LENS_WD_FIB_VIEW_FM uses the greater-than operator (>), which
incorrectly excludes objectives that have a working distance exactly equal to
the minimum threshold. Change the comparison operator from > to >= in the
condition checking lens.workingDistance.value to include objectives at the
minimum threshold and properly support FIB_VIEW_FM posture availability.
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 588-592: The RMS error calculation in the correlation result is
incorrect because the square root is being applied before the mean instead of
after. The current formula in the rms variable assignment computes the mean of
absolute differences rather than the true root-mean-square error. Fix this by
moving the square root operation to apply after the mean calculation instead of
before it - replace the current formula that has (** 2) ** 0.5 inside the mean
with a calculation that computes the mean of squared differences first, then
takes the square root of that result.
- Around line 201-202: In the math.isclose() call where fib_mill_angle and
z_stack_mill_angle are compared, change the parameter from
rel_tol=MILLING_ANGLE_TOLERANCE to abs_tol=MILLING_ANGLE_TOLERANCE. Since
MILLING_ANGLE_TOLERANCE is defined as an absolute angular threshold of 0.1
degrees, it should be passed as an absolute tolerance parameter to
math.isclose() rather than relative tolerance, so the angle matching comparison
uses a fixed angular delta regardless of the magnitude of the angles being
compared.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2377-2380: Add a guard to check if the MD_STAGE_POSITION_RAW key
exists in the metadata returned by getRawMetadata() before attempting to access
it, to prevent the code from failing when this metadata is missing.
Additionally, instead of using the current pm.milling_angle.value when
constructing the suffix, extract the milling angle from the stream's acquisition
pose or context (the angle at which the stream was actually acquired) and use
that value to label the stream name accurately, ensuring the label reflects the
stream's acquisition angle rather than the current stage angle.
In `@src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc`:
- Around line 71-95: The wxBoxSizer element that wraps the FM acquired posture
section is missing an explicit orient attribute, which can cause unintended
rendering behavior. Add an explicit `<orient>wxHORIZONTAL</orient>` element
inside the wxBoxSizer object to ensure the three inline elements (the "FM
acquired at" label, the bitmap icon, and the posture label) are properly stacked
horizontally, consistent with the layout pattern used throughout the file.
---
Outside diff comments:
In `@src/odemis/driver/static.py`:
- Around line 46-58: The OpticalLens.__init__ method is missing type annotations
for all parameters and lacks a return type hint. Convert the docstring from
inline type formatting (e.g., "name (string):", "mag (float > 0):") to
reStructuredText :param style without embedding type information in the
parameter descriptions. Add appropriate Python type hints to all parameters
including name, role, mag, mag_choices, na, ri, wd, pole_pos, mirror_pos_top,
mirror_pos_bottom, x_max, hole_diam, focus_dist, parabola_f, rotation,
configurations, and kwargs, then add a return type annotation of None to the
__init__ method signature.
---
Nitpick comments:
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 769-785: The method _update_overview_acquisition_button is calling
pm.at_fib_view_fm_posture(pm.stage.position.value) twice, which is inefficient.
Store the result of this method call in a local variable before using it in both
the enabled calculation and the tooltip condition, so the posture check is
performed only once instead of twice.
In `@src/odemis/gui/cont/multi_point_correlation.py`:
- Around line 544-545: The method _do_2d_correlation is missing an explicit
return type hint. Add `-> None` to the method signature after the closing
parenthesis of the parameter list to comply with the repository's typing
requirements, since this method does not return any value.
In `@src/odemis/gui/cont/tabs/localization_tab.py`:
- Around line 498-505: The posture indicator update logic in the if/elif block
only handles FIB_VIEW_FM and FM_IMAGING postures but lacks an else clause to
clear stale UI state when the stage moves to other postures. Add an else clause
after the elif statement that clears both self.panel.lbl_current_posture and
self.panel.bmp_current_posture by setting the label to an empty string and the
bitmap to an empty or default bitmap value to ensure the UI does not retain
outdated posture information.
🪄 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: bf803487-7af6-4965-a2e7-2138c5b06d25
⛔ Files ignored due to path filters (6)
src/odemis/gui/img/icon/ico_meteorimaging.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_orange.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_orange.pngis excluded by!**/*.png
📒 Files selected for processing (22)
install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yamlsrc/odemis/acq/feature.pysrc/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/acq/test/move_tfs3_test.pysrc/odemis/driver/static.pysrc/odemis/gui/comp/stream_panel.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcsrc/odemis/gui/xmlh/resources/panel_tab_localization.xrcsrc/odemis/model/_metadata.pysrc/odemis/odemisd/mdupdater.pyutil/groom-img.py
| if model.hasVA(lens, "workingDistance") and lens.workingDistance.value > MIN_LENS_WD_FIB_VIEW_FM: | ||
| self.has_fib_view_fm_objective = True |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an inclusive WD threshold for FIB_VIEW_FM compatibility.
MIN_LENS_WD_FIB_VIEW_FM is defined as a minimum; using > rejects objectives exactly at the threshold and can incorrectly disable FIB_VIEW_FM posture availability.
🐛 Proposed fix
- if model.hasVA(lens, "workingDistance") and lens.workingDistance.value > MIN_LENS_WD_FIB_VIEW_FM:
+ if model.hasVA(lens, "workingDistance") and lens.workingDistance.value >= MIN_LENS_WD_FIB_VIEW_FM:
self.has_fib_view_fm_objective = True📝 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.
| if model.hasVA(lens, "workingDistance") and lens.workingDistance.value > MIN_LENS_WD_FIB_VIEW_FM: | |
| self.has_fib_view_fm_objective = True | |
| if model.hasVA(lens, "workingDistance") and lens.workingDistance.value >= MIN_LENS_WD_FIB_VIEW_FM: | |
| self.has_fib_view_fm_objective = True |
🤖 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/acq/move.py` around lines 382 - 383, The condition comparing
lens.workingDistance.value against MIN_LENS_WD_FIB_VIEW_FM uses the greater-than
operator (>), which incorrectly excludes objectives that have a working distance
exactly equal to the minimum threshold. Change the comparison operator from > to
>= in the condition checking lens.workingDistance.value to include objectives at
the minimum threshold and properly support FIB_VIEW_FM posture availability.
| rms = numpy.mean(((fib_coords - fm_coords_transformed) ** 2) ** 0.5) | ||
| # Fill in error metric in similar way to 3d correlation, for consistency | ||
| self.correlation_target.correlation_result = { | ||
| "output": {"error": {"rms_error": rms}}, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RMS error calculation is currently not RMS.
The formula computes mean absolute component error, not root-mean-square distance per point. The displayed “RMS Deviation” will be incorrect.
💡 Proposed fix
- rms = numpy.mean(((fib_coords - fm_coords_transformed) ** 2) ** 0.5)
+ residuals = fib_coords - fm_coords_transformed
+ rms = numpy.sqrt(numpy.mean(numpy.sum(residuals ** 2, axis=1)))📝 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.
| rms = numpy.mean(((fib_coords - fm_coords_transformed) ** 2) ** 0.5) | |
| # Fill in error metric in similar way to 3d correlation, for consistency | |
| self.correlation_target.correlation_result = { | |
| "output": {"error": {"rms_error": rms}}, | |
| } | |
| residuals = fib_coords - fm_coords_transformed | |
| rms = numpy.sqrt(numpy.mean(numpy.sum(residuals ** 2, axis=1))) | |
| # Fill in error metric in similar way to 3d correlation, for consistency | |
| self.correlation_target.correlation_result = { | |
| "output": {"error": {"rms_error": rms}}, | |
| } |
🤖 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 588 - 592, The
RMS error calculation in the correlation result is incorrect because the square
root is being applied before the mean instead of after. The current formula in
the rms variable assignment computes the mean of absolute differences rather
than the true root-mean-square error. Fix this by moving the square root
operation to apply after the mean calculation instead of before it - replace the
current formula that has (** 2) ** 0.5 inside the mean with a calculation that
computes the mean of squared differences first, then takes the square root of
that result.
| pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW) | ||
| # Add milling angle suffix to stream's name if it was acquired at the milling angle. | ||
| if pm.at_fib_view_fm_posture(pos): | ||
| s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard missing stage metadata and compute the suffix angle from the stream pose.
This path can fail when MD_STAGE_POSITION_RAW is missing, and it currently labels using the current stage angle rather than the stream’s acquisition angle.
💡 Proposed fix
- pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW)
- # Add milling angle suffix to stream's name if it was acquired at the milling angle.
- if pm.at_fib_view_fm_posture(pos):
- s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°"
+ md = s.getRawMetadata()[0]
+ pos = md.get(model.MD_STAGE_POSITION_RAW) if md else None
+ # Add milling angle suffix to stream's name if it was acquired at the milling angle.
+ if isinstance(pos, dict) and pm.at_fib_view_fm_posture(pos):
+ rx = pos.get("rx")
+ if rx is not None:
+ milling_angle = pm.calculate_milling_angle(stage_tilt=rx, column_tilt=pm.fm_column_tilt)
+ s.name.value = f"{s.name.value} at {math.degrees(milling_angle):0.0f}°"🤖 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/stream_bar.py` around lines 2377 - 2380, Add a guard to
check if the MD_STAGE_POSITION_RAW key exists in the metadata returned by
getRawMetadata() before attempting to access it, to prevent the code from
failing when this metadata is missing. Additionally, instead of using the
current pm.milling_angle.value when constructing the suffix, extract the milling
angle from the stream's acquisition pose or context (the angle at which the
stream was actually acquired) and use that value to label the stream name
accurately, ensuring the label reflects the stream's acquisition angle rather
than the current stage angle.
| <object class="wxBoxSizer"> | ||
| <object class="sizeritem"> | ||
| <border>10</border> | ||
| <flag>wxRIGHT|wxALIGN_CENTER_VERTICAL</flag> | ||
| <object class="wxStaticText"> | ||
| <label>FM acquired at</label> | ||
| <fg>#DDDDDD</fg> | ||
| </object> | ||
| </object> | ||
| <object class="sizeritem"> | ||
| <object class="wxStaticBitmap" name="bmp_fm_posture"> | ||
| <bitmap>../../img/icon/ico_meteorimaging.png</bitmap> | ||
| <XRCED> | ||
| <assign_var>1</assign_var> | ||
| </XRCED> | ||
| </object> | ||
| </object> | ||
| <object class="sizeritem"> | ||
| <flag>wxALIGN_CENTER_VERTICAL</flag> | ||
| <object class="wxStaticText" name="lbl_fm_posture"> | ||
| <label>posture</label> | ||
| <fg>#777777</fg> | ||
| </object> | ||
| </object> | ||
| </object> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify wxBoxSizer entries missing <orient> in this XRC file.
python3 - <<'PY'
import xml.etree.ElementTree as ET
from pathlib import Path
path = Path("src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc")
root = ET.parse(path).getroot()
ns = {"x": "http://www.wxwidgets.org/wxxrc"}
missing = []
for obj in root.findall(".//x:object[`@class`='wxBoxSizer']", ns):
if obj.find("x:orient", ns) is None:
# Capture a nearby identifier when available
parent = None
missing.append("wxBoxSizer without orient found")
if missing:
print(f"{path}: {len(missing)} missing orient block(s)")
for m in missing:
print("-", m)
else:
print(f"{path}: all wxBoxSizer blocks define orient")
PYRepository: delmic/odemis
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned in the review to see exact context
sed -n '68,96p' src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcRepository: delmic/odemis
Length of output: 1503
🏁 Script executed:
#!/bin/bash
# Check other wxBoxSizer declarations to see what orient values they use
python3 - <<'PY'
import xml.etree.ElementTree as ET
from pathlib import Path
path = Path("src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc")
root = ET.parse(path).getroot()
ns = {"x": "http://www.wxwidgets.org/wxxrc"}
print("All wxBoxSizer declarations in file:")
for i, obj in enumerate(root.findall(".//x:object[`@class`='wxBoxSizer']", ns), 1):
orient_elem = obj.find("x:orient", ns)
if orient_elem is not None:
orient = orient_elem.text
else:
orient = "MISSING"
# Try to find a nearby name to identify the sizer
print(f" {i}. orient={orient}")
PYRepository: delmic/odemis
Length of output: 375
Add explicit orientation to the new wxBoxSizer.
Line 71 introduces a wxBoxSizer without <orient>, which may cause unintended rendering behavior. The new row contains three inline elements (label, bitmap, label) requiring horizontal stacking. Add an explicit <orient>wxHORIZONTAL</orient> to match the intended layout and align with the pattern used by other sizers in this file.
Suggested fix
<object class="sizeritem">
<border>10</border>
<flag>wxALL</flag>
<object class="wxBoxSizer">
+ <orient>wxHORIZONTAL</orient>
<object class="sizeritem">
<border>10</border>
<flag>wxRIGHT|wxALIGN_CENTER_VERTICAL</flag>🤖 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/xmlh/resources/dialog_correlation_tdct.xrc` around lines 71 -
95, The wxBoxSizer element that wraps the FM acquired posture section is missing
an explicit orient attribute, which can cause unintended rendering behavior. Add
an explicit `<orient>wxHORIZONTAL</orient>` element inside the wxBoxSizer object
to ensure the three inline elements (the "FM acquired at" label, the bitmap
icon, and the posture label) are properly stacked horizontally, consistent with
the layout pattern used throughout the file.
tmoerkerken
force-pushed
the
fm-milling-correlation
branch
from
c78a066 to
daea76c
Compare
tmoerkerken
force-pushed
the
fm-milling-correlation
branch
from
daea76c to
353924d
Compare
|
I have a general question. Why do we do 2d correlation from the multi point correlation dialog box instead of using correlation tab which is designed to do 2D? Maybe @pieleric knows it. |
It's a decision I made, based on a discussion between Eric and Marit. Either would have been possible, but since I think point matching is more accurate, reusing the existing point matching widget was the quickest implementation. By the way, I do think we need to critically look at the two, once we do the redesign. They probably should be one tool. Eric: Marit: |
| if pm.at_fib_view_fm_posture(pos): | ||
| s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°" | ||
| if self.tab_data_model.main.currentFeature.value: | ||
| s.name.value = self.tab_data_model.main.currentFeature.value.name.value + " - " + s.name.value |
There was a problem hiding this comment.
Is this the intention, to remove the feature name from the stream name?
There was a problem hiding this comment.
Yes, that was the plan. We only show streams corresponding to the selected feature. So it's kind of redundant. Plus, we had to make a bit of room for the "at 10°" suffix.
There was a problem hiding this comment.
I understand why it is removed, because as we display streams for the current feature, it does not make sense to add feature name. Although this is true only for the Localization Tab. The feature name is helpful to be added in the stream name when the the users view diffferent features together in the Gallery/Analysis/Odemis Viewer.
There was a problem hiding this comment.
Yes, I can see how that is helpful. The current master code also does not show the feature name prefix in the Analysis/Odemis Viewer tabs, so maybe add it to the idea board?
| """ | ||
| Called when the stage is moved, enable the tab if position is imaging mode, disable otherwise | ||
| Called when the stage is moved, and perform the following actions: | ||
| - enable the tab if position is imaging mode, disable otherwise |
There was a problem hiding this comment.
We can maybe explicit which imaging mode we are talking about, is it overview acquisition?
There was a problem hiding this comment.
Yeah, this docstring is not great. In the other PR I left a comment to improve it. Should be FM Imaging and FIB-view FM
|
|
||
| from odemis.acq.align.transform import CalculateTransform | ||
|
|
||
| from odemis.gui.img import getBitmap | ||
|
|
||
| from odemis.acq.move import POSITION_NAMES, FM_IMAGING, FIB_VIEW_FM | ||
|
|
There was a problem hiding this comment.
The blank lines should not be there between the imports
| # i.e. size in pixel[0]=pixel[1]=pixel[2]. | ||
| COM_ROI_PADDING = 12 # Padding (pixels) for center of mass ROI extraction | ||
| REFINE_SEARCH_RANGE = 2.5e-6 # m, search range for fiducial refinement | ||
| # TODO: Could adapt padding based on pixel spacing for more flexibility if needed |
There was a problem hiding this comment.
Is this comment still valid?
There was a problem hiding this comment.
No, that was exactly what was implemented now, so should be gone 😅 .
| if pm.at_fib_view_fm_posture(pos): | ||
| s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°" | ||
| if self.tab_data_model.main.currentFeature.value: | ||
| s.name.value = self.tab_data_model.main.currentFeature.value.name.value + " - " + s.name.value |
There was a problem hiding this comment.
I understand why it is removed, because as we display streams for the current feature, it does not make sense to add feature name. Although this is true only for the Localization Tab. The feature name is helpful to be added in the stream name when the the users view diffferent features together in the Gallery/Analysis/Odemis Viewer.
|
|
||
| # Hide the z-column for the FIB-view FM workflow, since we only perform 2d correlation. | ||
| if self.at_fib_view_fm.value: | ||
| self.hide_grid_column(3) |
There was a problem hiding this comment.
to avoid using hard coded numbers
| self.hide_grid_column(3) | |
| self.hide_grid_column(GridColumns.Z.value) |
There was a problem hiding this comment.
Nice, much better
| self._panel = frame | ||
| self._viewports = frame.pnl_correlation_grid.viewports | ||
|
|
||
| streams = self._main_data_model.currentFeature.value.streams.value |
There was a problem hiding this comment.
There is a possibility that the current features has both the streams, aka, FM streams and FM at milling angle. If there are both streams then the lines from 210 to 218, will just focus on FM at milling angle, and not consider the FM view at all. Should this not be decided by the user, if they want to use milling angle FM or normal FM ?
If the user had created z stack at FM view, they might want to use the information
There was a problem hiding this comment.
Indeed, we decided that FIB-view FM takes priority. But I do agree it can be confusing to the user. We made the decision also based on the knowledge that these are two different workflows, and in practice, having the two flavours of FM would be rare. We could however make use of the visibility, to have some form of control. I will have a look.
There was a problem hiding this comment.
Alright, I made some changes (there was a bug in stream deletion). Now you can delete the undesired stream from the localization tab, and it will not use it for correlation. If I'm correct, this would be an edge-case though.
| pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW) | ||
| # Add milling angle suffix to stream's name if it was acquired at the milling angle. | ||
| if pm.at_fib_view_fm_posture(pos): | ||
| s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°" |
There was a problem hiding this comment.
Why do we add this twice, once in the locatlzation tab and also here?
There was a problem hiding this comment.
When freshly acquiring the data, we use a different display method, since the data is already in memory (odemis.gui.cont.tabs.localization_tab.LocalizationTab.display_acquired_data) then when (lazy) loading the data from disk (odemis.gui.cont.stream_bar.CryoAcquiredStreamsController._display_feature_streams).
It does look a bit strange that we have a CryoAcquiredStreamsController but separately also handling it in the LocalizationTab. That's part of the reason I'm going to look into this for a while in upcoming sprints, since it all feels a bit incoherent.
tmoerkerken
force-pushed
the
fm-milling-correlation
branch
from
353924d to
c4f0127
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2251-2259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
streams.value.remove()against streams not owned by the current feature.
_on_user_stream_removeruns for any removable stream, including overview streams.remove_imagesearches by filename and tolerates absence, butlist.remove(stream)raisesValueErrorwhen the removed stream isn't inself._current_feature.streams.value(e.g., removing an overview stream while a feature is selected), crashing the handler beforesave_project.🛡️ Proposed guard
if self._current_feature: remove_image(self._current_feature.images.value, md[MD_FILENAME], [md[MD_IN_FILE_INDEX]]) - self._current_feature.streams.value.remove(stream) + if stream in self._current_feature.streams.value: + self._current_feature.streams.value.remove(stream)🤖 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/stream_bar.py` around lines 2251 - 2259, Guard the stream removal in _on_user_stream_remove so it only calls self._current_feature.streams.value.remove(stream) when the stream is actually owned by the current feature. The issue is that overview streams can reach this handler while a feature is selected, and list.remove will raise ValueError if the stream is not present. Update the logic around _on_user_stream_remove and the current feature’s streams collection to check membership (or otherwise confirm ownership) before removing, while still leaving the remove_image calls and save_project flow intact.
🤖 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.
Outside diff comments:
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2251-2259: Guard the stream removal in _on_user_stream_remove so
it only calls self._current_feature.streams.value.remove(stream) when the stream
is actually owned by the current feature. The issue is that overview streams can
reach this handler while a feature is selected, and list.remove will raise
ValueError if the stream is not present. Update the logic around
_on_user_stream_remove and the current feature’s streams collection to check
membership (or otherwise confirm ownership) before removing, while still leaving
the remove_image calls and save_project flow intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c30722c-e400-4810-9cd2-a8ba906edc55
📒 Files selected for processing (10)
src/odemis/acq/move.pysrc/odemis/gui/comp/stream_panel.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/multi_point_correlation.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrcsrc/odemis/gui/xmlh/resources/panel_tab_localization.xrc
✅ Files skipped from review due to trivial changes (2)
- src/odemis/acq/move.py
- src/odemis/gui/comp/stream_panel.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc
- src/odemis/gui/xmlh/resources/panel_tab_localization.xrc
- src/odemis/gui/cont/features.py
- src/odemis/gui/cont/acquisition/cryo_acq.py
- src/odemis/gui/cont/tabs/localization_tab.py
tmoerkerken
force-pushed
the
fm-milling-correlation
branch
from
c4f0127 to
76a56aa
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the cryo/Meteor correlation workflow to better support FIB-view FM data by detecting the FM acquisition posture, adjusting UI indicators, and switching correlation behavior (2D vs 3D) and requirements accordingly.
Changes:
- Add posture/FIB-view FM indicators to the localization tab and correlation dialog UI.
- Update correlation logic to support a simplified 2D workflow for FIB-view FM (including hiding the Z column and reducing required fiducials).
- Fix stream deletion bookkeeping and adjust stream naming/display to include milling-angle context.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/gui/xmlh/resources/panel_tab_localization.xrc | Adds “Current posture” indicator controls to the localization panel UI. |
| src/odemis/gui/xmlh/resources/dialog_correlation_tdct.xrc | Adds FM posture indicator controls to the correlation dialog UI. |
| src/odemis/gui/main_xrc.py | Updates generated XRC bindings/resources to expose new posture indicator controls and embedded icons. |
| src/odemis/gui/cont/tabs/localization_tab.py | Enables localization tab for FM + FIB-view FM and updates current posture indicator; adds milling-angle stream name suffix on acquisition. |
| src/odemis/gui/cont/stream_bar.py | Fixes deletion bookkeeping; appends milling-angle suffix when displaying feature streams. |
| src/odemis/gui/cont/multi_point_correlation.py | Adds FIB-view FM detection, 2D correlation path, hides Z column in that workflow, and refactors refinement control enabling. |
| src/odemis/gui/cont/features.py | Adds FIB_VIEW_FM to supported postures list. |
| src/odemis/gui/cont/acquisition/cryo_acq.py | Disables overview acquisition at FIB-view FM posture and centralizes button enable/tooltip logic. |
| src/odemis/gui/comp/stream_panel.py | Changes stream name ellipsizing to middle to better preserve suffix/prefix visibility. |
| src/odemis/acq/move.py | Minor comment cleanup in posture switching logic. |
| @@ -32,23 +33,27 @@ | |||
|
|
|||
| import numpy | |||
| import wx | |||
| # origin of the stream instead, but just trying both simplifies things. | ||
| if self._current_feature: | ||
| remove_image(self._current_feature.images.value, md[MD_FILENAME], [md[MD_IN_FILE_INDEX]]) | ||
| self._current_feature.streams.value.remove(stream) |
| pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW) | ||
| # Add milling angle suffix to stream's name if it was acquired at the milling angle. | ||
| if pm.at_fib_view_fm_posture(pos): | ||
| s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°" |
| if self.main_data.posture_manager.at_fib_view_fm_posture(pos): | ||
| self.panel.lbl_current_posture.SetLabel(POSITION_NAMES[FIB_VIEW_FM]) | ||
| self.panel.bmp_current_posture.SetBitmap(self.bmp_fib_view_fm) | ||
| elif self.main_data.posture_manager.at_fm_imaging_posture(pos): | ||
| self.panel.lbl_current_posture.SetLabel(POSITION_NAMES[FM_IMAGING]) | ||
| self.panel.bmp_current_posture.SetBitmap(self.bmp_fm_imaging) |
| @call_in_wx_main | ||
| def _update_refine_controls(self) -> None: | ||
| if self.at_fib_view_fm.value: | ||
| self.xyz_targeting_btn.Enable(False) | ||
| self.txt_refine_xyz_active.SetLabel("") | ||
| self.xyz_targeting_btn.SetToolTip("Refinement disabled when using FIB-view FM streams") | ||
| elif self.has_super_z.value: | ||
| self.xyz_targeting_btn.SetToolTip("Super Z information available, Refinement disabled") | ||
| self.xyz_targeting_btn.Enable(False) | ||
| self.txt_refine_xyz_active.SetLabel("Super Z information in use") | ||
| elif TargetType.FibFiducial == self._tab_data_model.main.currentTarget.value.type.value: | ||
| self.xyz_targeting_btn.Enable(False) | ||
| self.txt_refine_xyz_active.SetLabel("") | ||
| self.xyz_targeting_btn.SetToolTip("Refinement only available for non-reflective FM streams") | ||
| else: | ||
| self.xyz_targeting_btn.Enable(True) | ||
| self.txt_refine_xyz_active.SetLabel("") | ||
| self.xyz_targeting_btn.SetToolTip("Refine the position of the currently selected FM fiducial") | ||
|
|
| rms = numpy.mean(((fib_coords - fm_coords_transformed) ** 2) ** 0.5) | ||
| # Fill in error metric in similar way to 3d correlation, for consistency | ||
| self.correlation_target.correlation_result = { | ||
| "output": {"error": {"rms_error": rms}}, | ||
| } |
Warning
Requires #3492 to be merged, so the diff is showing a lot of unrelated files. The most important change is src/odemis/gui/cont/multi_point_correlation.py
The PR implements changes to facilitate a correlation workflow for FIB-view FM data: