[MSD-265][fix] METEOR GUI FIBSEM tab: untangle milling pattern handling#3383
Open
pieleric wants to merge 1 commit intodelmic:masterfrom
Open
[MSD-265][fix] METEOR GUI FIBSEM tab: untangle milling pattern handling#3383pieleric wants to merge 1 commit intodelmic:masterfrom
pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a per-task Sequence DiagramsequenceDiagram
actor User
participant UI as Milling UI
participant Controller as MillingTaskController
participant Feature as CryoFeature
participant TaskModel as MillingTaskSettings
participant Storage as save_features
User->>UI: Toggle task checkbox / select task
UI->>Controller: _on_selected_tasks(selected_list)
Controller->>Feature: update feature.milling_tasks (mark selected)
Controller->>Storage: save_features(Feature)
Controller->>Controller: _update_pattern_panels()
Controller->>UI: show/hide panels
User->>UI: Click to move patterns
UI->>Controller: move_milling_tasks(relative_pos)
Controller->>Feature: load feature.reference_image
Controller->>TaskModel: update pattern centers (pos_to_relative/pos_to_absolute)
Controller->>Storage: save_features(Feature)
Controller->>Controller: draw_milling_tasks()
Controller->>UI: render shapes for selected tasks
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (2)
src/odemis/acq/milling/test/tasks_test.py (1)
92-92: Consider adding test coverage forselected=Falsescenarios.The test validates the default
selected=Truecase, but additional test cases would strengthen coverage:
- Creating a task with
selected=Falseand verifyingto_dict()output- Deserializing a dict with
"selected": Falseand verifying the attribute- Verifying
generate()returns an empty list whenselected=FalseWould you like me to generate additional test cases for these scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/milling/test/tasks_test.py` at line 92, Add tests covering the selected=False path: create a Task instance with selected=False and assert task.to_dict()["selected"] is False; deserialize a dict with "selected": False via Task.from_dict(...) (or the module's deserialization helper) and assert the resulting object's selected attribute is False; and assert that calling the instance's generate(...) method returns an empty list when selected is False. Use the existing test file's patterns/setup and reference the Task class and its to_dict, from_dict (or equivalent deserializer), and generate methods to locate where to add these assertions.src/odemis/gui/cont/acquisition/cryo_acq.py (1)
809-828: Consider adding a guard forfeature.reference_image.While
btn_tdctis only enabled whenreference_image is not None(line 217), adding a defensive check would prevent potentialAttributeErrorif this code path is reached through other means in the future.Optional defensive check
if correlation_dict and correlation_dict.fib_projected_pois: if correlation_dict.fm_pois: # Update feature position according to POI in FM ... # Draw milling position in FIBSEM tab around the projected POI target = correlation_dict.fib_projected_pois[0] + if feature.reference_image is None: + logging.warning("Cannot move milling tasks: no reference image") + return rel_pos = pos_to_relative(target.coordinates.value[:2], feature.reference_image) fibsem_tab.milling_task_controller.move_milling_tasks(rel_pos)🤖 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 809 - 828, Add a defensive guard to ensure feature.reference_image is not None before using it: in the block in cryo_acq.py where you compute rel_pos = pos_to_relative(target.coordinates.value[:2], feature.reference_image) (and then call fibsem_tab.milling_task_controller.move_milling_tasks), check that feature.reference_image is present (and skip or early-return if not) to avoid AttributeError; keep all existing FM/FIB handling (feature.posture_positions, feature.fm_focus_position) but only call pos_to_relative and move_milling_tasks when feature.reference_image is non-null.
🤖 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/milling.py`:
- Around line 104-113: The function rectangle_pattern_to_shape currently uses an
implicit Optional for the parameter `name: str = None`; update the signature to
use an explicit optional type (e.g. `name: Optional[str]` or `name: str | None`)
and add the corresponding import from typing (`Optional`) or use PEP 604 union
syntax if the project targets Python 3.10+. Change the
`rectangle_pattern_to_shape` parameter annotation accordingly and update any
related type hints/usages in the file to remain consistent.
---
Nitpick comments:
In `@src/odemis/acq/milling/test/tasks_test.py`:
- Line 92: Add tests covering the selected=False path: create a Task instance
with selected=False and assert task.to_dict()["selected"] is False; deserialize
a dict with "selected": False via Task.from_dict(...) (or the module's
deserialization helper) and assert the resulting object's selected attribute is
False; and assert that calling the instance's generate(...) method returns an
empty list when selected is False. Use the existing test file's patterns/setup
and reference the Task class and its to_dict, from_dict (or equivalent
deserializer), and generate methods to locate where to add these assertions.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 809-828: Add a defensive guard to ensure feature.reference_image
is not None before using it: in the block in cryo_acq.py where you compute
rel_pos = pos_to_relative(target.coordinates.value[:2], feature.reference_image)
(and then call fibsem_tab.milling_task_controller.move_milling_tasks), check
that feature.reference_image is present (and skip or early-return if not) to
avoid AttributeError; keep all existing FM/FIB handling
(feature.posture_positions, feature.fm_focus_position) but only call
pos_to_relative and move_milling_tasks when feature.reference_image is non-null.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/milling/millmng.pysrc/odemis/acq/milling/tasks.pysrc/odemis/acq/milling/test/tasks_test.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/milling.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/features.py
There was a problem hiding this comment.
Pull request overview
This PR fixes several issues with milling pattern handling in the METEOR GUI FIBSEM tab. The core problem was that changing features would revert pattern center positions to (0,0), only selected patterns were stored in JSON (causing position issues when selecting new patterns), and pattern settings could disappear or be modified even without a feature selected.
Changes:
- Added a
selectedboolean attribute toMillingTaskSettingsto track which tasks should be executed, allowing all patterns to be saved to JSON regardless of selection state - Refactored position conversion functions (
pos_to_relativeandpos_to_absolute) to accept a reference image directly instead of a stream, separating concerns between drawing, moving, and coordinate conversion - Reorganized the
MillingTaskControllerto independently listen to feature changes and fully update pattern lists and settings when features change, eliminating attempts to detect unchanged patterns
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/acq/milling/tasks.py | Added selected boolean attribute to MillingTaskSettings class with serialization support and generation filtering |
| src/odemis/acq/milling/test/tasks_test.py | Updated test to verify the new selected attribute is properly serialized |
| src/odemis/acq/milling/millmng.py | Modified get_associated_tasks to filter out unselected milling tasks |
| src/odemis/gui/cont/milling.py | Major refactoring: renamed position conversion functions, separated move_milling_tasks from draw_milling_tasks, added independent feature change listener, simplified task selection handling |
| src/odemis/gui/cont/features.py | Removed save_milling_tasks method that was filtering tasks, as this is now handled by the selected attribute |
| src/odemis/gui/cont/acquisition/cryo_acq.py | Updated to use new move_milling_tasks API with relative position conversion |
src/odemis/gui/cont/milling.py
Outdated
Comment on lines
+215
to
+218
| def _update_pattern_panels(self): | ||
| """ | ||
| Update the pattern settings control, when a new feature is selected. | ||
| :return: |
There was a problem hiding this comment.
The docstring for _update_pattern_panels has an empty :return: field that should either be removed or completed with a description of what the function returns. Since the function doesn't return anything, the :return: line should be removed.
Suggested change
| def _update_pattern_panels(self): | |
| """ | |
| Update the pattern settings control, when a new feature is selected. | |
| :return: | |
| def _update_pattern_panels(self) -> None: | |
| """ | |
| Update the pattern settings control, when a new feature is selected. |
pieleric
force-pushed
the
fix-meteor-gui-milling-pattern-connection
branch
from
a6aab92 to
b354132
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/milling.py (1)
76-96:⚠️ Potential issue | 🟡 MinorUse defensive metadata access pattern consistent with codebase conventions.
Both
pos_to_relativeandpos_to_absoluteaccessref_img.metadata[model.MD_POS]using direct dictionary access. This is inconsistent with the established codebase pattern, which uses.get(model.MD_POS, default_value)throughout similar code (e.g.,stream_bar.py,correlation.py,multi_point_correlation.py). If the metadata key is missing, direct access will raise aKeyError. While callers check thatreference_image is not None, they don't validate thatMD_POSexists. Consider using.get()with an appropriate default or documenting the precondition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 76 - 96, Replace direct dict access to ref_img.metadata[model.MD_POS] in pos_to_relative and pos_to_absolute with the defensive pattern used elsewhere: use ref_img.metadata.get(model.MD_POS, (0.0, 0.0)) to obtain stream_pos, then proceed with the arithmetic; additionally validate the retrieved stream_pos is a 2-tuple of numbers and raise a clear ValueError (or return a sensible fallback) if it is not, so callers won't get a KeyError when MD_POS is missing or malformed.
♻️ Duplicate comments (1)
src/odemis/gui/cont/milling.py (1)
99-103:⚠️ Potential issue | 🟡 MinorFix implicit
Optionaltype hint.Per PEP 484 and coding guidelines,
name: str = Noneshould use explicit union syntax for Python 3.10+.Proposed fix
def rectangle_pattern_to_shape(canvas, ref_img: model.DataArray, pattern: RectanglePatternParameters, colour: str = "#FFFF00", - name: str = None) -> EditableShape: + name: str | None = None) -> EditableShape:,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 99 - 103, The function rectangle_pattern_to_shape has an implicit Optional type for the parameter "name" (declared as name: str = None); update the type hint to an explicit union form supported in Python 3.10+ by changing the signature to use "str | None" (i.e., name: str | None = None) so the parameter's optionality is explicit; keep the rest of the signature and defaults unchanged and run type checks to confirm.
🧹 Nitpick comments (7)
src/odemis/gui/cont/milling.py (7)
307-330: Add type hint and docstring.Per coding guidelines, this method should have parameter type hints and a docstring.
Proposed fix
- def on_mouse_down(self, evt): + def on_mouse_down(self, evt: wx.MouseEvent) -> None: + """Handle mouse down event for moving milling patterns.""" active_canvas = evt.GetEventObject()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 307 - 330, Add a parameter type hint and a short docstring to the on_mouse_down method: annotate the evt parameter with the appropriate wx event type (e.g., wx.MouseEvent or the project's event alias) and add a one- or two-line docstring at the top of on_mouse_down explaining its purpose (handles mouse down, checks modifiers, converts view to physical coords, and moves milling tasks) and describing the evt parameter and return behavior; ensure references to active_canvas, feature, move_milling_tasks, and pos_to_relative remain unchanged and that the method still calls evt.Skip() when not handling the event.
390-393: Add type hints.The
_parameter (likely for callback compatibility) and return type should have type hints.Proposed fix
`@call_in_wx_main` - def draw_milling_tasks(self, _=None): - """Redraw all milling tasks on the canvas. - """ + def draw_milling_tasks(self, _: object = None) -> None: + """Redraw all milling tasks on the canvas."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 390 - 393, The draw_milling_tasks function decorated with `@call_in_wx_main` should include type hints for its callback parameter and return type: add a type for the `_` parameter (e.g. `_: Optional[Any] = None`) and a return annotation `-> None`; also import Optional and Any from typing if not already present so the signature becomes def draw_milling_tasks(_ : Optional[Any] = None) -> None while preserving the `@call_in_wx_main` decorator and docstring.
332-336: Add return type hint.Per coding guidelines, all functions should have return type hints.
Proposed fix
`@call_in_wx_main` - def set_milling_tasks(self, milling_tasks: Dict[str, MillingTaskSettings]): + def set_milling_tasks(self, milling_tasks: Dict[str, MillingTaskSettings]) -> None: """ Sets the milling tasks displayed to the provided ones """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 332 - 336, The method set_milling_tasks decorated with `@call_in_wx_main` is missing a return type hint; add an explicit return annotation (-> None) to the function signature for set_milling_tasks(self, milling_tasks: Dict[str, MillingTaskSettings]) to comply with the guidelines, and ensure any linter/type-checker imports (e.g., typing.Dict) remain valid for the annotated signature.
377-388: Add return type hint.Proposed fix
- def move_milling_tasks(self, pos: Tuple[float, float]): + def move_milling_tasks(self, pos: Tuple[float, float]) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 377 - 388, The method move_milling_tasks is missing an explicit return type hint; update its signature to include a return type (-> None) since it mutates state and does not return a value. Modify the def move_milling_tasks(self, pos: Tuple[float, float]) to def move_milling_tasks(self, pos: Tuple[float, float]) -> None, leaving the body (iterating over self.milling_tasks, setting pattern.center.value, calling save_features and draw_milling_tasks) unchanged.
200-207: Add return type hint.Per coding guidelines, all functions should have return type hints.
Proposed fix
- def _on_current_feature_changes(self, feature: Optional[CryoFeature]): + def _on_current_feature_changes(self, feature: Optional[CryoFeature]) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 200 - 207, The method _on_current_feature_changes is missing a return type hint; update its signature to include an explicit return type (-> None) so it reads def _on_current_feature_changes(self, feature: Optional[CryoFeature]) -> None:, leaving the body unchanged and keeping references to set_milling_tasks and _update_pattern_panels intact.
367-376: Add return type hint and docstring.Per coding guidelines,
_on_selected_tasksshould have a return type hint and docstring.Proposed fix
- def _on_selected_tasks(self, tasks: List[str]): + def _on_selected_tasks(self, tasks: List[str]) -> None: + """Update task selection states and persist to file.""" if self._tab_data.main.currentFeature.value is None: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 367 - 376, Add a return type hint and a docstring to the _on_selected_tasks method: add "-> None" to the signature and insert a brief triple-quoted docstring immediately under the def that describes the purpose (update task selection based on provided task names), the parameter (tasks: List[str]) and side-effects (saves features via save_features and updates UI via draw_milling_tasks). Keep the docstring one to three sentences and mention that it early-returns if no current feature is selected to clarify behavior.
210-214: Add return type hint.Per coding guidelines, all functions should have return type hints.
Proposed fix
`@call_in_wx_main` - def _update_pattern_panels(self): + def _update_pattern_panels(self) -> None: """ Update the pattern settings control, when a new feature is selected. - :return: """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 210 - 214, The method _update_pattern_panels is missing a return type hint; update its signature in class Milling (or wherever defined) to include an explicit return type (use -> None for a method that does not return a value) so it conforms to the coding guidelines and static type checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 76-96: Replace direct dict access to
ref_img.metadata[model.MD_POS] in pos_to_relative and pos_to_absolute with the
defensive pattern used elsewhere: use ref_img.metadata.get(model.MD_POS, (0.0,
0.0)) to obtain stream_pos, then proceed with the arithmetic; additionally
validate the retrieved stream_pos is a 2-tuple of numbers and raise a clear
ValueError (or return a sensible fallback) if it is not, so callers won't get a
KeyError when MD_POS is missing or malformed.
---
Duplicate comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 99-103: The function rectangle_pattern_to_shape has an implicit
Optional type for the parameter "name" (declared as name: str = None); update
the type hint to an explicit union form supported in Python 3.10+ by changing
the signature to use "str | None" (i.e., name: str | None = None) so the
parameter's optionality is explicit; keep the rest of the signature and defaults
unchanged and run type checks to confirm.
---
Nitpick comments:
In `@src/odemis/gui/cont/milling.py`:
- Around line 307-330: Add a parameter type hint and a short docstring to the
on_mouse_down method: annotate the evt parameter with the appropriate wx event
type (e.g., wx.MouseEvent or the project's event alias) and add a one- or
two-line docstring at the top of on_mouse_down explaining its purpose (handles
mouse down, checks modifiers, converts view to physical coords, and moves
milling tasks) and describing the evt parameter and return behavior; ensure
references to active_canvas, feature, move_milling_tasks, and pos_to_relative
remain unchanged and that the method still calls evt.Skip() when not handling
the event.
- Around line 390-393: The draw_milling_tasks function decorated with
`@call_in_wx_main` should include type hints for its callback parameter and return
type: add a type for the `_` parameter (e.g. `_: Optional[Any] = None`) and a
return annotation `-> None`; also import Optional and Any from typing if not
already present so the signature becomes def draw_milling_tasks(_ :
Optional[Any] = None) -> None while preserving the `@call_in_wx_main` decorator
and docstring.
- Around line 332-336: The method set_milling_tasks decorated with
`@call_in_wx_main` is missing a return type hint; add an explicit return
annotation (-> None) to the function signature for set_milling_tasks(self,
milling_tasks: Dict[str, MillingTaskSettings]) to comply with the guidelines,
and ensure any linter/type-checker imports (e.g., typing.Dict) remain valid for
the annotated signature.
- Around line 377-388: The method move_milling_tasks is missing an explicit
return type hint; update its signature to include a return type (-> None) since
it mutates state and does not return a value. Modify the def
move_milling_tasks(self, pos: Tuple[float, float]) to def
move_milling_tasks(self, pos: Tuple[float, float]) -> None, leaving the body
(iterating over self.milling_tasks, setting pattern.center.value, calling
save_features and draw_milling_tasks) unchanged.
- Around line 200-207: The method _on_current_feature_changes is missing a
return type hint; update its signature to include an explicit return type (->
None) so it reads def _on_current_feature_changes(self, feature:
Optional[CryoFeature]) -> None:, leaving the body unchanged and keeping
references to set_milling_tasks and _update_pattern_panels intact.
- Around line 367-376: Add a return type hint and a docstring to the
_on_selected_tasks method: add "-> None" to the signature and insert a brief
triple-quoted docstring immediately under the def that describes the purpose
(update task selection based on provided task names), the parameter (tasks:
List[str]) and side-effects (saves features via save_features and updates UI via
draw_milling_tasks). Keep the docstring one to three sentences and mention that
it early-returns if no current feature is selected to clarify behavior.
- Around line 210-214: The method _update_pattern_panels is missing a return
type hint; update its signature in class Milling (or wherever defined) to
include an explicit return type (use -> None for a method that does not return a
value) so it conforms to the coding guidelines and static type checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/milling/millmng.pysrc/odemis/acq/milling/tasks.pysrc/odemis/acq/milling/test/tasks_test.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/milling.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/features.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/odemis/gui/cont/acquisition/cryo_acq.py
- src/odemis/acq/milling/test/tasks_test.py
- src/odemis/acq/milling/millmng.py
- src/odemis/acq/milling/tasks.py
There were several issues with the milling patterns: * changing feature reverts the center position of all the patterns to 0,0 * Only the patterns selected for milling were stored in json, so selecting a new one would add it to the wrong position * Could end up in situations where a pattern setting is not shown anymore for a given feature * If no feature is selected, it was still possible to change the pattern settings... but that had no effect. In order to fix these issues, all at once, the pattern management scheme has been reorganised: * the milling task has an extra attribute "selected" which indicates whether it should be milling or not. It's recorded to JSON, and so all patterns can be saved to JSON, selected or not. * separate drawing a pattern from moving a pattern, from converting between relative and absolute position. * The MillingTaskController is independently listening to the current feature change (not relying on MillingTaskController to call it) * Whenever a feature is changed, the list of patterns and settings is entirely updated. No attempt to optimize by detecting that the patterns are the same as the previous one.
pieleric
force-pushed
the
fix-meteor-gui-milling-pattern-connection
branch
from
b354132 to
8f1120d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/odemis/acq/milling/test/tasks_test.py (1)
92-92: Consider adding test coverage forselectededge cases.The assertion for
to_dict()is good, but consider adding tests for:
from_dict()with explicitselected=Falsefrom_dict()without"selected"key (backward compatibility)generate()returning an empty list whenselected=False💡 Suggested additional test cases
def test_milling_task_settings_selected_false(self): milling_settings = MillingSettings(100e-9, 30e3, 400e-6, "Serial", "ion") trench_pattern = TrenchPatternParameters(1e-6, 1e-6, 100e-9, 1e-6, (0, 0)) # Test with selected=False task = MillingTaskSettings(milling_settings, [trench_pattern], "Task", selected=False) self.assertFalse(task.selected) self.assertEqual(task.to_dict()["selected"], False) # generate() should return empty list when not selected self.assertEqual(task.generate(), []) def test_milling_task_settings_from_dict_backward_compat(self): # Test backward compatibility: no "selected" key defaults to True data = { "name": "Legacy Task", "milling": {"current": 100e-9, "voltage": 30e3, "field_of_view": 400e-6}, "patterns": [] } task = MillingTaskSettings.from_dict(data) self.assertTrue(task.selected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/milling/test/tasks_test.py` at line 92, Add tests covering selected edge cases for MillingTaskSettings: create a MillingTaskSettings instance with selected=False (use MillingSettings and TrenchPatternParameters as in existing tests), assert task.selected is False, assert to_dict()["selected"] is False, and assert generate() returns an empty list when selected is False; also add a backward-compatibility test that calls MillingTaskSettings.from_dict() with a dict missing the "selected" key (e.g., data with "name", "milling", and "patterns" only) and assert the resulting task.selected defaults to True. Ensure tests reference MillingTaskSettings.from_dict, .to_dict, and .generate methods explicitly.src/odemis/gui/cont/milling.py (1)
337-341: Consider using equality check instead of identity check.Using
iscompares object identity, which may fail ifmilling_tasksis a new dictionary with the same content (e.g., after deserialization). If the intent is to detect when the same dictionary object is passed, this is fine; otherwise, consider a content-based comparison.💡 Alternative using equality or explicit no-change detection
If the goal is to skip updates when tasks haven't changed:
- if self.milling_tasks is milling_tasks: + if self.milling_tasks is milling_tasks: # Skip if exact same objectIf content comparison is needed, you'd need a deeper equality check, but the current approach is acceptable if tasks are always passed by reference from the feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/gui/cont/milling.py` around lines 337 - 341, The code currently uses an identity check ("if self.milling_tasks is milling_tasks:") which misses cases where a new but equal-magnitude object is passed; change this to a content-based comparison (e.g., "if self.milling_tasks == milling_tasks:") or, if nested structures require it, use a deep equality check (e.g., compare via json.dumps or deepcopy-aware comparator) in the method where self.milling_tasks is assigned so the update is skipped when contents are equal while still updating when a genuinely different task set is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/odemis/acq/milling/test/tasks_test.py`:
- Line 92: Add tests covering selected edge cases for MillingTaskSettings:
create a MillingTaskSettings instance with selected=False (use MillingSettings
and TrenchPatternParameters as in existing tests), assert task.selected is
False, assert to_dict()["selected"] is False, and assert generate() returns an
empty list when selected is False; also add a backward-compatibility test that
calls MillingTaskSettings.from_dict() with a dict missing the "selected" key
(e.g., data with "name", "milling", and "patterns" only) and assert the
resulting task.selected defaults to True. Ensure tests reference
MillingTaskSettings.from_dict, .to_dict, and .generate methods explicitly.
In `@src/odemis/gui/cont/milling.py`:
- Around line 337-341: The code currently uses an identity check ("if
self.milling_tasks is milling_tasks:") which misses cases where a new but
equal-magnitude object is passed; change this to a content-based comparison
(e.g., "if self.milling_tasks == milling_tasks:") or, if nested structures
require it, use a deep equality check (e.g., compare via json.dumps or
deepcopy-aware comparator) in the method where self.milling_tasks is assigned so
the update is skipped when contents are equal while still updating when a
genuinely different task set is provided.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/milling/millmng.pysrc/odemis/acq/milling/tasks.pysrc/odemis/acq/milling/test/tasks_test.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/features.pysrc/odemis/gui/cont/milling.py
💤 Files with no reviewable changes (1)
- src/odemis/gui/cont/features.py
tmoerkerken
reviewed
Mar 2, 2026
| :return: List of associated tasks.""" | ||
| associated_tasks = [] | ||
| for task in milling_tasks.values(): | ||
| if not task.selected: |
Contributor
There was a problem hiding this comment.
@ilyushkin does the addition of the selected key have any implications for FibsemOS? (not sure if there is a strict format).
pieleric
changed the title
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.
There were several issues with the milling patterns:
In order to fix these issues, all at once, the pattern management scheme has been reorganised: