[MSD-39][feature] Adjust the Odemis FIBSEM milling tab to Tescan settings #3305
Conversation
📝 WalkthroughWalkthroughThe PR adds milling configuration and parameter propagation: new MillingConfig (file-backed) with rate, dwell_time, and fibsemos config_path; GUI and CryoGUIData obtain and pass milling_conf into CryoFeature creation; load_milling_tasks accepts general_params and applies them to tasks; MillingSettings gains rate and dwell_time fields; a config_path parameter is threaded through microscope factory functions, FibsemOS/TFSMilling/Automated managers, and run_milling_tasks_fibsemos. StaticFIBStream is used for FIB acquisition routing. Minor API changes include CryoGUIData.add_new_feature f_name and updated public signatures that accept config_path/general_params. Sequence Diagram(s)mermaid Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/odemis/acq/milling/test/tasks_test.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/conf/test/conf_test.py (1)
65-68: AddCONF_MILLINGreset totearDownto prevent test pollution.The
tearDownmethod resetsCONF_GENERAL,CONF_ACQUI, andCONF_CALIB, but does not reset the newCONF_MILLING. This could cause test ordering issues if tests run in a different sequence.# Reset the module globals gui.conf.CONF_GENERAL = None gui.conf.CONF_ACQUI = None gui.conf.CONF_CALIB = None + gui.conf.CONF_MILLING = None
🧹 Nitpick comments (6)
src/odemis/acq/milling/fibsemos.py (1)
69-81: Consider using explicitOptionaltype hint forconfig_path.The parameter
config_path: Path = Noneuses implicit Optional, which is discouraged by PEP 484. For consistency and clarity, consider using explicit optional typing.-def create_fibsemos_tescan_microscope(config_path: Path = None) -> 'OdemisTescanMicroscope': +def create_fibsemos_tescan_microscope(config_path: Path | None = None) -> 'OdemisTescanMicroscope':The same applies to
create_fibsemos_microscope(line 100),FibsemOSMillingTaskManager.__init__(line 236), andrun_milling_tasks_fibsemos(line 314).src/odemis/acq/milling/tasks.py (1)
127-127: Avoid mutable default argument.Using
{}as a default argument can lead to unexpected behavior if the dict is mutated, since the same object is reused across calls. While the current implementation doesn't mutate the parameter itself, it's safer to useNoneand initialize inside the function.-def load_milling_tasks(path: str, general_params = {}) -> Dict[str, MillingTaskSettings]: +def load_milling_tasks(path: str, general_params: dict = None) -> Dict[str, MillingTaskSettings]: """Load milling tasks from a yaml file. :param path: path to the yaml file :param general_params: general params for all the tasks :return: dictionary of milling tasks """ + if general_params is None: + general_params = {} milling_tasks = {}src/odemis/acq/feature.py (1)
152-154: Avoid mutable default argument.Using
{}as a default argument is flagged by static analysis (B006). While the current usage doesn't mutate the dict, it's best practice to useNoneand initialize inside.def __init__(self, name: str, stage_position: Dict[str, float], fm_focus_position: Dict[str, float], streams: Optional[List[Stream]] = None, milling_tasks: Optional[Dict[str, MillingTaskSettings]] = None, correlation_data = None, - general_params = {}): + general_params: Optional[Dict] = None): """ ... """ + if general_params is None: + general_params = {} self.name = model.StringVA(name)src/odemis/acq/milling/millmng.py (3)
306-306: Use explicitOptionaltype annotation.PEP 484 recommends explicit
Optional[Path]orPath | Noneinstead of implicit optional via default value.+from typing import Dict, List, Optional + class AutomatedMillingManager(object): def __init__(self, future: Future, features: List[CryoFeature], stage: model.Actuator, sem_stream: SEMStream, fib_stream: FIBStream, task_list: List[MillingWorkflowTask], - config_path: Path=None + config_path: Optional[Path] = None ):
509-509: Use explicitOptionaltype annotation.Same issue as above for consistency.
def run_automated_milling(features: List[CryoFeature], stage: model.Actuator, sem_stream: SEMStream, fib_stream: FIBStream, task_list: List[MillingWorkflowTask], - config_path: Path=None + config_path: Optional[Path] = None ) -> Future:
376-379: Commented-out code with TODO-style note.There's commented-out alignment code at line 379 with inconsistent indentation. If the alignment step is intentionally disabled, consider adding a clear TODO comment explaining when it should be re-enabled, or remove the dead code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml(1 hunks)src/odemis/acq/feature.py(2 hunks)src/odemis/acq/milling/fibsemos.py(8 hunks)src/odemis/acq/milling/millmng.py(8 hunks)src/odemis/acq/milling/tasks.py(3 hunks)src/odemis/gui/conf/__init__.py(3 hunks)src/odemis/gui/conf/data.py(1 hunks)src/odemis/gui/conf/file.py(1 hunks)src/odemis/gui/conf/test/conf_test.py(1 hunks)src/odemis/gui/cont/milling.py(7 hunks)src/odemis/gui/cont/stream_bar.py(2 hunks)src/odemis/gui/model/tab_gui_data.py(3 hunks)src/odemis/util/dataio.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/gui/model/tab_gui_data.py
📚 Learning: 2025-10-20T10:02:02.606Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:360-379
Timestamp: 2025-10-20T10:02:02.606Z
Learning: In the cryo workflow, POIs (Points of Interest) are linked to features and should always be derived from the latest feature position and focus values when loading correlation data, rather than using any saved POI values from correlation_data.fm_pois. This ensures POIs reflect the current feature state.
Applied to files:
src/odemis/gui/model/tab_gui_data.py
🧬 Code graph analysis (9)
src/odemis/gui/cont/stream_bar.py (1)
src/odemis/acq/stream/_static.py (3)
StaticSEMStream(321-340)StaticFIBStream(343-351)StaticStream(51-115)
src/odemis/gui/conf/test/conf_test.py (2)
src/odemis/gui/conf/file.py (7)
MillingConfig(578-617)rate(596-597)rate(600-601)dwell_time(604-605)dwell_time(608-609)config_path(612-613)config_path(616-617)src/odemis/gui/conf/__init__.py (1)
get_milling_conf(71-77)
src/odemis/gui/conf/__init__.py (1)
src/odemis/gui/conf/file.py (1)
MillingConfig(578-617)
src/odemis/acq/milling/millmng.py (1)
src/odemis/acq/milling/fibsemos.py (1)
run_milling_tasks_fibsemos(312-336)
src/odemis/acq/milling/fibsemos.py (2)
src/odemis/acq/milling/tasks.py (1)
MillingSettings(36-73)src/odemis/model/_futures.py (1)
ProgressiveFuture(371-507)
src/odemis/acq/feature.py (1)
src/odemis/acq/milling/tasks.py (2)
MillingTaskSettings(76-114)load_milling_tasks(127-146)
src/odemis/gui/model/tab_gui_data.py (2)
src/odemis/gui/conf/__init__.py (2)
get_general_conf(36-41)get_milling_conf(71-77)src/odemis/acq/feature.py (1)
CryoFeature(144-242)
src/odemis/util/dataio.py (1)
src/odemis/acq/stream/_static.py (1)
StaticFIBStream(343-351)
src/odemis/gui/conf/data.py (1)
src/odemis/gui/conf/util.py (1)
resolution_from_range(56-86)
🪛 GitHub Actions: Linting
src/odemis/gui/conf/__init__.py
[error] 77-77: flake8: W292 no newline at end of file
🪛 Ruff (0.14.8)
src/odemis/acq/milling/millmng.py
306-306: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
509-509: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
src/odemis/acq/milling/fibsemos.py
69-69: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
236-236: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
314-314: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
src/odemis/acq/feature.py
154-154: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/acq/milling/tasks.py
127-127: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/gui/conf/data.py
953-953: Dictionary key literal "ion-beam" repeated
(F601)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (19)
install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml (1)
88-88: Formatting alignment with project conventions.Reformatting the numeric literal to the shortened exponent form (
-40.e-3vs.-40.e-03) improves consistency with other values in the file and aligns with standard Python scientific notation conventions.src/odemis/gui/cont/stream_bar.py (2)
39-39: LGTM!The import of
StaticFIBStreamis correctly added to support the FIB stream handling in theCryoFIBAcquiredStreamsController.
2286-2288: LGTM!The type check correctly filters for
StaticFIBStreaminstances, aligning with the controller's FIB-specific purpose and the PR objective to use static FIB streams for the cryo feature view.src/odemis/util/dataio.py (1)
129-131: LGTM!The new branch correctly routes FIB acquisition data (
MD_AT_FIB) toStaticFIBStream, following the established pattern for other acquisition types in this function.src/odemis/acq/milling/fibsemos.py (3)
158-186: LGTM!The
_format_presetfunction correctly formats voltage and current with appropriate unit scaling. The keV notation for voltage follows common electron microscopy conventions.
233-248: LGTM!The
config_pathparameter is correctly propagated to the microscope creation, with appropriate documentation.
312-325: LGTM!The
config_pathparameter is correctly added and propagated through toFibsemOSMillingTaskManager.src/odemis/gui/conf/__init__.py (1)
71-77: LGTM with minor fix needed: add newline at end of file.The
get_milling_conf()function correctly follows the established pattern for lazy-loading configuration objects. However, the pipeline failure indicates a missing newline at the end of the file.def get_milling_conf(): """ Return the Milling config object and create/read it first if it does not yet exist """ global CONF_MILLING if not CONF_MILLING: from .file import MillingConfig CONF_MILLING = MillingConfig() return CONF_MILLING +src/odemis/gui/conf/test/conf_test.py (1)
217-248: LGTM!The
MillingConfigTestclass follows the established patterns for config testing in this file. The tests cover type validation, persistence (save/reload), and default values correctly.src/odemis/gui/model/tab_gui_data.py (2)
263-263: LGTM!Initializing
milling_confvia lazy accessor follows the same pattern asget_general_conf()and other configuration accessors in the codebase.
290-296: LGTM!The
general_paramsdictionary correctly packagesrateanddwell_timefrom the milling configuration and passes them toCryoFeature, enabling consistent milling parameter propagation.src/odemis/acq/milling/tasks.py (2)
39-44: LGTM!The
MillingSettingsclass is properly extended withrateanddwell_timeparameters. TheFloatContinuousranges (0 to 1.0 for rate, 0 to 60s for dwell_time) are reasonable bounds.
63-70: Consider whether default values of 0 forrateanddwell_timeare appropriate.When loading from a dict that lacks
rateordwell_time, these default to0. Given that the system-wide defaults are1e-8for rate and1e-6for dwell_time (fromMillingConfig), a value of0could indicate an uninitialized state or cause unexpected behavior in downstream calculations.Please verify that
rate=0anddwell_time=0are valid operational values, or consider using the same defaults asMillingConfig.src/odemis/gui/conf/file.py (1)
578-617: LGTM!The
MillingConfigclass follows the established configuration pattern in this file. The property accessors correctly handle type conversion (float for rate/dwell_time, str for config_path), and default values are sensible.src/odemis/acq/feature.py (1)
169-170: LGTM!The
general_paramsis correctly propagated toload_milling_taskswhen default milling tasks need to be loaded.src/odemis/acq/milling/millmng.py (1)
429-429: LGTM!The
config_pathis correctly propagated torun_milling_tasks_fibsemoswhen fibsemos is installed.src/odemis/gui/cont/milling.py (3)
156-165: LGTM!Clear separation of acquisition config (
acqui_conf) and milling config (milling_conf). Thegeneral_paramsconstruction matches the pattern intab_gui_data.py, ensuring consistency.
449-453: LGTM!The conditional branching between
run_milling_tasks_fibsemos(when fibsemos is installed) andmillmng.run_milling_tasks(fallback) is a clean integration pattern. Theconfig_pathis correctly passed to the fibsemos path.
663-670: LGTM!The
config_pathis properly retrieved frommilling_confand passed through torun_automated_milling, completing the configuration propagation chain.
ilyushkin
force-pushed
the
msd-39-adjust-odemis-to-tescan-settings
branch
from
a57a746 to
2872690
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/odemis/acq/milling/fibsemos.py (1)
189-199: Tests need to be updated for new parameters.The existing tests for
convert_milling_settingsdon't passrateanddwell_timetoMillingSettings, and don't validate the newrate,dwell_time, andpresetfields in the converted output.This was already flagged in a previous review. The tests should be updated to include the new required parameters and assertions.
🧹 Nitpick comments (4)
src/odemis/acq/milling/millmng.py (2)
306-306: Use explicitOptionalor union type forconfig_pathparameter.Per PEP 484,
Path = Noneimplies an optional type but doesn't declare it explicitly. This can cause issues with type checkers.- config_path: Path=None + config_path: Optional[Path] = NoneNote: You'll need to import
Optionalfromtyping(already imported in this file based on line 35).
506-506: Same type annotation issue forconfig_pathinrun_automated_milling.Apply the same fix for consistency with the class parameter.
- config_path: Path=None + config_path: Optional[Path] = Nonesrc/odemis/acq/milling/fibsemos.py (2)
69-69: Use explicitOptionaltype annotation forconfig_path.Per PEP 484, using
Path = Nonewithout explicitOptionalis discouraged.-def create_fibsemos_tescan_microscope(config_path: Path = None) -> 'OdemisTescanMicroscope': +def create_fibsemos_tescan_microscope(config_path: Optional[Path] = None) -> 'OdemisTescanMicroscope':Note: Add
Optionalto the imports fromtypingon line 9.
100-100: Apply consistentOptionaltype annotations throughout.Same issue applies to
create_fibsemos_microscope,FibsemOSMillingTaskManager.__init__, andrun_milling_tasks_fibsemos.-def create_fibsemos_microscope(config_path: Path = None) -> 'OdemisThermoMicroscope | OdemisTescanMicroscope': +def create_fibsemos_microscope(config_path: Optional[Path] = None) -> 'OdemisThermoMicroscope | OdemisTescanMicroscope':- config_path: Path = None): + config_path: Optional[Path] = None):- config_path: Path = None) -> futures.Future: + config_path: Optional[Path] = None) -> futures.Future:Also applies to: 236-236, 313-314
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/odemis/acq/feature.py(2 hunks)src/odemis/acq/milling/fibsemos.py(8 hunks)src/odemis/acq/milling/millmng.py(8 hunks)src/odemis/acq/milling/tasks.py(3 hunks)src/odemis/gui/cont/milling.py(7 hunks)src/odemis/gui/model/tab_gui_data.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/model/tab_gui_data.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/odemis/acq/feature.py (1)
src/odemis/acq/milling/tasks.py (2)
MillingTaskSettings(76-114)load_milling_tasks(127-146)
src/odemis/gui/cont/milling.py (6)
src/odemis/acq/milling/fibsemos.py (1)
run_milling_tasks_fibsemos(312-336)src/odemis/acq/milling/patterns.py (1)
RectanglePatternParameters(56-98)src/odemis/acq/milling/tasks.py (2)
MillingTaskSettings(76-114)load_milling_tasks(127-146)src/odemis/gui/comp/overlay/shapes.py (2)
EditableShape(47-170)ShapesOverlay(173-551)src/odemis/gui/conf/__init__.py (2)
get_acqui_conf(53-59)get_milling_conf(71-77)src/odemis/acq/milling/millmng.py (1)
run_milling_tasks(250-269)
src/odemis/acq/milling/tasks.py (1)
src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
src/odemis/acq/milling/fibsemos.py (1)
src/odemis/acq/milling/tasks.py (1)
MillingSettings(36-73)
src/odemis/acq/milling/millmng.py (1)
src/odemis/acq/milling/fibsemos.py (1)
run_milling_tasks_fibsemos(312-336)
🪛 Ruff (0.14.8)
src/odemis/acq/feature.py
154-154: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/acq/milling/tasks.py
127-127: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/acq/milling/fibsemos.py
69-69: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
236-236: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
314-314: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
src/odemis/acq/milling/millmng.py
306-306: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
506-506: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (10)
src/odemis/acq/milling/millmng.py (1)
426-426: LGTM!The
config_pathis correctly propagated torun_milling_tasks_fibsemoswhen fibsemOS is installed.src/odemis/acq/feature.py (1)
169-170: LGTM!The
general_paramsis correctly forwarded toload_milling_taskswhen loading default milling tasks.src/odemis/acq/milling/tasks.py (3)
39-44: LGTM!The new
rateanddwell_timeparameters are properly added toMillingSettingswith appropriate units and ranges.
63-70: LGTM!The
from_dictmethod correctly handles backward compatibility with default values of0forrateanddwell_time.
137-141: Consider whetherupdateshould overwrite existing values.Using
milling.update(general_params)will overwrite any task-specificrateordwell_timevalues defined in the YAML file with the global defaults. If task-specific values should take precedence, reverse the merge order.If global params should only fill in missing values (not override), consider:
merged = {**general_params, **milling} # task values override globals task_data["milling"] = mergedsrc/odemis/gui/cont/milling.py (3)
156-165: LGTM!The milling configuration is properly initialized and
general_paramsis correctly constructed from the milling config values.
449-453: LGTM!The conditional path selection between fibsemOS and legacy milling is correctly implemented, with
config_pathproperly passed to the fibsemOS path.
561-562: LGTM!The
AutomatedMillingControllercorrectly retrieves both acquisition and milling configurations and properly passesconfig_pathtorun_automated_milling.Also applies to: 669-669
src/odemis/acq/milling/fibsemos.py (2)
158-186: LGTM!The
_format_presethelper is well-implemented with appropriate unit scaling for voltage (keV) and current (pA/nA/uA).
108-111: The asymmetry is intentional and correct by design.The
create_fibsemos_tfs_microscope()function loads the default microscope configuration viaload_microscope_configuration()without arguments, whilecreate_fibsemos_tescan_microscope()loads a custom configuration from a provided path. This difference reflects that TFS uses a default config, whereas TESCAN requires a configurable path.
| milling_tasks: Optional[Dict[str, MillingTaskSettings]] = None, | ||
| correlation_data = None, | ||
| general_params = {}): |
There was a problem hiding this comment.
Mutable default argument will cause shared state bugs.
Using general_params = {} as a default creates a single dict object shared across all calls. If any code modifies this dict, it affects all subsequent instantiations.
def __init__(self, name: str,
stage_position: Dict[str, float],
fm_focus_position: Dict[str, float],
streams: Optional[List[Stream]] = None,
milling_tasks: Optional[Dict[str, MillingTaskSettings]] = None,
- correlation_data = None,
- general_params = {}):
+ correlation_data = None,
+ general_params: Optional[Dict] = None):Then initialize inside the method:
if general_params is None:
general_params = {}🧰 Tools
🪛 Ruff (0.14.8)
154-154: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In src/odemis/acq/feature.py around lines 152-154, the function signature uses a
mutable default argument `general_params = {}`, which leads to shared-state
bugs; change the default to `general_params=None` and inside the function check
`if general_params is None:` then assign a new empty dict (or a copy of a
provided dict) before using it, ensuring each call gets its own dictionary.
| yaml.dump(mdict, f) | ||
|
|
||
| def load_milling_tasks(path: str) -> Dict[str, MillingTaskSettings]: | ||
| def load_milling_tasks(path: str, general_params = {}) -> Dict[str, MillingTaskSettings]: |
There was a problem hiding this comment.
Mutable default argument on general_params.
Same issue as in feature.py - the empty dict default is shared across calls.
-def load_milling_tasks(path: str, general_params = {}) -> Dict[str, MillingTaskSettings]:
+def load_milling_tasks(path: str, general_params: Optional[Dict] = None) -> Dict[str, MillingTaskSettings]:Then at the start of the function:
if general_params is None:
general_params = {}Note: Add Optional to imports from typing.
🧰 Tools
🪛 Ruff (0.14.8)
127-127: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
In src/odemis/acq/milling/tasks.py around line 127, the function
load_milling_tasks uses a mutable default argument (general_params = {}), which
shares state across calls; change the signature to accept Optional[Dict[str,
Any]] (i.e., general_params: Optional[Dict[str, Any]] = None), update the early
guard to set general_params = {} when None, and add Optional (and any needed
typing imports like Dict, Any) to the file's typing imports.
ilyushkin
force-pushed
the
msd-39-adjust-odemis-to-tescan-settings
branch
from
2872690 to
ff8637f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/odemis/acq/feature.py (1)
152-154: Mutable default argumentgeneral_params = {}will cause shared state bugs.This issue was already flagged in a previous review. Using a mutable default creates a single dict shared across all calls.
src/odemis/acq/milling/tasks.py (1)
127-127: Mutable default argumentgeneral_params = {}will cause shared state bugs.This issue was already flagged in a previous review.
🧹 Nitpick comments (6)
src/odemis/gui/conf/test/conf_test.py (1)
213-248: AddCONF_MILLINGreset to the baseConfigTest.tearDownmethod.The
MillingConfigTestclass correctly follows the existing test patterns. However, the baseConfigTest.tearDown(lines 66-68) resetsCONF_GENERAL,CONF_ACQUI, andCONF_CALIBbut doesn't includeCONF_MILLING. This could cause test pollution.Add the reset in
tearDown:# Reset the module globals gui.conf.CONF_GENERAL = None gui.conf.CONF_ACQUI = None gui.conf.CONF_CALIB = None + gui.conf.CONF_MILLING = Nonesrc/odemis/acq/milling/fibsemos.py (3)
69-69: Use explicit type union for optional parameters.PEP 484 prohibits implicit
Optional. Update the type hints to use explicitPath | NoneorOptional[Path].Apply this diff:
-def create_fibsemos_tescan_microscope(config_path: Path = None) -> 'OdemisTescanMicroscope': +def create_fibsemos_tescan_microscope(config_path: Path | None = None) -> 'OdemisTescanMicroscope':-def create_fibsemos_microscope(config_path: Path = None) -> 'OdemisThermoMicroscope | OdemisTescanMicroscope': +def create_fibsemos_microscope(config_path: Path | None = None) -> 'OdemisThermoMicroscope | OdemisTescanMicroscope':Also applies to: 100-100
236-236: Use explicit type union for optional parameter.PEP 484 prohibits implicit
Optional. Update the type hint to use explicitPath | None.Apply this diff:
- config_path: Path = None): + config_path: Path | None = None):
314-314: Use explicit type union for optional parameter.PEP 484 prohibits implicit
Optional. Update the type hint to use explicitPath | None.Apply this diff:
- config_path: Path = None) -> futures.Future: + config_path: Path | None = None) -> futures.Future:src/odemis/acq/milling/millmng.py (2)
306-306: Use explicit type union for optional parameter.PEP 484 prohibits implicit
Optional. Update the type hint to use explicitPath | None.Apply this diff:
- config_path: Path=None + config_path: Path | None = None
506-506: Use explicit type union for optional parameter.PEP 484 prohibits implicit
Optional. Update the type hint to use explicitPath | None.Apply this diff:
- config_path: Path=None + config_path: Path | None = None
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/odemis/acq/feature.py(2 hunks)src/odemis/acq/milling/fibsemos.py(8 hunks)src/odemis/acq/milling/millmng.py(8 hunks)src/odemis/acq/milling/tasks.py(3 hunks)src/odemis/gui/conf/__init__.py(3 hunks)src/odemis/gui/conf/file.py(1 hunks)src/odemis/gui/conf/test/conf_test.py(1 hunks)src/odemis/gui/cont/milling.py(7 hunks)src/odemis/gui/cont/stream_bar.py(2 hunks)src/odemis/gui/model/tab_gui_data.py(3 hunks)src/odemis/util/dataio.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/odemis/util/dataio.py
- src/odemis/gui/cont/stream_bar.py
- src/odemis/gui/conf/file.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/gui/model/tab_gui_data.py
📚 Learning: 2025-10-20T10:02:02.606Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:360-379
Timestamp: 2025-10-20T10:02:02.606Z
Learning: In the cryo workflow, POIs (Points of Interest) are linked to features and should always be derived from the latest feature position and focus values when loading correlation data, rather than using any saved POI values from correlation_data.fm_pois. This ensures POIs reflect the current feature state.
Applied to files:
src/odemis/gui/model/tab_gui_data.py
🧬 Code graph analysis (7)
src/odemis/gui/cont/milling.py (3)
src/odemis/acq/milling/fibsemos.py (1)
run_milling_tasks_fibsemos(312-336)src/odemis/gui/conf/__init__.py (1)
get_milling_conf(71-77)src/odemis/acq/milling/millmng.py (1)
run_milling_tasks(250-269)
src/odemis/gui/conf/test/conf_test.py (2)
src/odemis/gui/conf/file.py (7)
MillingConfig(578-617)rate(596-597)rate(600-601)dwell_time(604-605)dwell_time(608-609)config_path(612-613)config_path(616-617)src/odemis/gui/conf/__init__.py (1)
get_milling_conf(71-77)
src/odemis/gui/conf/__init__.py (1)
src/odemis/gui/conf/file.py (1)
MillingConfig(578-617)
src/odemis/gui/model/tab_gui_data.py (2)
src/odemis/gui/conf/__init__.py (2)
get_general_conf(36-41)get_milling_conf(71-77)src/odemis/acq/feature.py (1)
CryoFeature(144-242)
src/odemis/acq/milling/fibsemos.py (1)
src/odemis/acq/milling/tasks.py (1)
MillingSettings(36-73)
src/odemis/acq/milling/tasks.py (1)
src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
src/odemis/acq/milling/millmng.py (1)
src/odemis/acq/milling/fibsemos.py (1)
run_milling_tasks_fibsemos(312-336)
🪛 Ruff (0.14.8)
src/odemis/acq/milling/fibsemos.py
69-69: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
100-100: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
236-236: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
314-314: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
src/odemis/acq/milling/tasks.py
127-127: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/acq/feature.py
154-154: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
src/odemis/acq/milling/millmng.py
306-306: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
506-506: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (12)
src/odemis/gui/conf/__init__.py (1)
71-77: LGTM!The
get_milling_conf()function follows the established lazy-loading pattern used by other configuration accessors in this module.src/odemis/gui/model/tab_gui_data.py (2)
263-263: LGTM!Loading the milling configuration once in
CryoGUIData.__init__and storing it as an instance attribute is appropriate for sharing configuration across feature operations.
291-296: Good approach for passing milling parameters.Constructing
general_paramsfrom the configuration and passing it explicitly toCryoFeatureavoids coupling the feature class directly to the configuration system.src/odemis/acq/milling/tasks.py (2)
65-66: Default values of 0 may be inconsistent with intended behavior.The
from_dictmethod defaultsrateanddwell_timeto 0 when not present in the data. However, theMillingConfigclass (infile.py) defines defaults of1e-8for rate and1e-6for dwell_time. A value of 0 for rate or dwell_time may cause issues in milling calculations.Consider aligning these defaults with the configuration values or documenting why 0 is acceptable for deserialization.
137-141: Theupdatecall overwrites task-specific values with general params.Using
milling.update(general_params)will overwrite any task-specificrateordwell_timevalues already defined in the YAML file with the general params. If the intent is to use general params only as defaults (fallback when not specified per-task), the merge order should be reversed:- for _, task_data in yaml_file.items(): - milling = task_data.get("milling", {}) - milling.update(general_params) - task_data["milling"] = milling + for _, task_data in yaml_file.items(): + milling = task_data.get("milling", {}) + # Apply general_params as defaults, task-specific values take precedence + merged = {**general_params, **milling} + task_data["milling"] = mergedIf overwriting is intentional (general params should always take precedence), please confirm this is the expected behavior.
src/odemis/acq/milling/fibsemos.py (2)
158-186: LGTM! Clean unit formatting logic.The
_format_presetfunction correctly scales voltage to keV and current to appropriate units (pA/nA/uA) with clear thresholds and formatting.
189-199: LGTM! Milling settings conversion correctly maps new fields.The conversion now includes
rate,dwell_time, and dynamically generatedpresetstring, properly mapping fromMillingSettingstoFibsemMillingSettings.src/odemis/acq/milling/millmng.py (1)
414-431: LGTM! Config path correctly propagated to fibsemOS path.The milling task execution correctly passes
config_pathto the fibsemOS path while maintaining the legacy TFS path without it.src/odemis/gui/cont/milling.py (4)
156-165: LGTM! Milling configuration properly integrated.The controller correctly retrieves the milling configuration and applies
rateanddwell_timeas general parameters to all loaded milling tasks.
448-454: LGTM! Clean path selection for milling execution.The code correctly selects between the fibsemOS path (with
config_path) and the legacy TFS path based on availability, with proper parameter propagation.
539-539: LGTM! Improved text consistency.The change to "No tasks selected..." improves consistency with typical UI text conventions.
561-562: LGTM! Config path properly integrated into automated milling workflow.The automated milling controller correctly retrieves configuration and propagates
config_paththrough the automated milling workflow, consistent with the manual milling path.Also applies to: 642-642, 669-669
ilyushkin
requested review from
tmoerkerken
and removed request for
K4rishma
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/odemis/acq/milling/test/tasks_test.py (3)
45-80: Add test coverage for rate and dwell_time fields.The test creates
MillingSettingswithrateanddwell_timeparameters but doesn't verify them. Please add assertions to check:
- Direct value access (lines 62-66):
self.assertEqual(milling_settings.rate.value, 0.0)andself.assertEqual(milling_settings.dwell_time.value, 0.0)- Serialization (lines 68-73):
self.assertEqual(dict_data["rate"], 0.0)andself.assertEqual(dict_data["dwell_time"], 0.0)- Deserialization (lines 75-80):
self.assertEqual(milling_settings_from_dict.rate.value, 0.0)andself.assertEqual(milling_settings_from_dict.dwell_time.value, 0.0)🔎 Apply this diff to add missing assertions:
self.assertEqual(milling_settings.current.value, current) self.assertEqual(milling_settings.voltage.value, voltage) self.assertEqual(milling_settings.field_of_view.value, field_of_view) + self.assertEqual(milling_settings.rate.value, 0.0) + self.assertEqual(milling_settings.dwell_time.value, 0.0) self.assertEqual(milling_settings.mode.value, mode) self.assertEqual(milling_settings.channel.value, channel) dict_data = milling_settings.to_dict() self.assertEqual(dict_data["current"], current) self.assertEqual(dict_data["voltage"], voltage) self.assertEqual(dict_data["field_of_view"], field_of_view) + self.assertEqual(dict_data["rate"], 0.0) + self.assertEqual(dict_data["dwell_time"], 0.0) self.assertEqual(dict_data["mode"], mode) self.assertEqual(dict_data["channel"], channel) milling_settings_from_dict = MillingSettings.from_dict(dict_data) self.assertEqual(milling_settings_from_dict.current.value, current) self.assertEqual(milling_settings_from_dict.voltage.value, voltage) self.assertEqual(milling_settings_from_dict.field_of_view.value, field_of_view) + self.assertEqual(milling_settings_from_dict.rate.value, 0.0) + self.assertEqual(milling_settings_from_dict.dwell_time.value, 0.0) self.assertEqual(milling_settings_from_dict.mode.value, mode) self.assertEqual(milling_settings_from_dict.channel.value, channel)
82-122: Add test coverage for rate and dwell_time in MillingTaskSettings.The test doesn't verify that
rateanddwell_timeare correctly handled inMillingTaskSettings. Please add assertions after line 100 and line 117 to check these fields are preserved through serialization and deserialization.🔎 Apply this diff to add missing assertions:
self.assertEqual(milling_task_settings.milling.current.value, milling_settings.current.value) self.assertEqual(milling_task_settings.milling.voltage.value, milling_settings.voltage.value) self.assertEqual(milling_task_settings.milling.field_of_view.value, milling_settings.field_of_view.value) + self.assertEqual(milling_task_settings.milling.rate.value, milling_settings.rate.value) + self.assertEqual(milling_task_settings.milling.dwell_time.value, milling_settings.dwell_time.value) self.assertEqual(milling_task_settings.milling.mode.value, milling_settings.mode.value) self.assertEqual(milling_task_settings.milling.channel.value, milling_settings.channel.value) self.assertEqual(milling_task_settings.patterns[0].width.value, trench_pattern.width.value)And after line 117:
self.assertEqual(milling_task_settings_from_dict.milling.current.value, milling_settings.current.value) self.assertEqual(milling_task_settings_from_dict.milling.voltage.value, milling_settings.voltage.value) self.assertEqual(milling_task_settings_from_dict.milling.field_of_view.value, milling_settings.field_of_view.value) + self.assertEqual(milling_task_settings_from_dict.milling.rate.value, milling_settings.rate.value) + self.assertEqual(milling_task_settings_from_dict.milling.dwell_time.value, milling_settings.dwell_time.value) self.assertEqual(milling_task_settings_from_dict.milling.mode.value, milling_settings.mode.value) self.assertEqual(milling_task_settings_from_dict.milling.channel.value, milling_settings.channel.value) self.assertEqual(milling_task_settings_from_dict.patterns[0].width.value, trench_pattern.width.value)
124-178: Verify rate and dwell_time persistence in save/load test.The test saves and loads milling tasks but doesn't verify that
rateanddwell_timeare correctly persisted to the YAML file and loaded back. Please add assertions to check these fields for both the "Trench" and "Microexpansion" tasks.🔎 Apply this diff to add missing assertions:
After line 162 (for Trench task):
self.assertEqual(loaded_tasks["Trench"].milling.current.value, trench_task_settings.milling.current.value) self.assertEqual(loaded_tasks["Trench"].milling.voltage.value, trench_task_settings.milling.voltage.value) self.assertEqual(loaded_tasks["Trench"].milling.field_of_view.value, trench_task_settings.milling.field_of_view.value) + self.assertEqual(loaded_tasks["Trench"].milling.rate.value, trench_task_settings.milling.rate.value) + self.assertEqual(loaded_tasks["Trench"].milling.dwell_time.value, trench_task_settings.milling.dwell_time.value) self.assertEqual(loaded_tasks["Trench"].milling.mode.value, trench_task_settings.milling.mode.value) self.assertEqual(loaded_tasks["Trench"].milling.channel.value, trench_task_settings.milling.channel.value)After line 173 (for Microexpansion task):
self.assertEqual(loaded_tasks["Microexpansion"].milling.current.value, microexpansion_task_settings.milling.current.value) self.assertEqual(loaded_tasks["Microexpansion"].milling.voltage.value, microexpansion_task_settings.milling.voltage.value) self.assertEqual(loaded_tasks["Microexpansion"].milling.field_of_view.value, microexpansion_task_settings.milling.field_of_view.value) + self.assertEqual(loaded_tasks["Microexpansion"].milling.rate.value, microexpansion_task_settings.milling.rate.value) + self.assertEqual(loaded_tasks["Microexpansion"].milling.dwell_time.value, microexpansion_task_settings.milling.dwell_time.value) self.assertEqual(loaded_tasks["Microexpansion"].milling.mode.value, microexpansion_task_settings.milling.mode.value) self.assertEqual(loaded_tasks["Microexpansion"].milling.channel.value, microexpansion_task_settings.milling.channel.value)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/milling/test/tasks_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/milling/test/tasks_test.py (1)
src/odemis/acq/milling/tasks.py (2)
MillingSettings(36-73)MillingTaskSettings(76-114)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
src/odemis/acq/milling/fibsemos.py
Outdated
| rate=s.rate.value, # um^3/nA/s | ||
| dwell_time=s.dwell_time.value # us |
There was a problem hiding this comment.
In Odemis, we normally always have the values stored without prefix. So the dwell time should be in "s", and the and rate in "m³/A/s". If fibsemOS uses different units, please convert here.
| tasks: List[MillingTaskSettings], | ||
| path: Optional[str] = None): | ||
| path: Optional[str] = None, | ||
| config_path: Path = None): |
There was a problem hiding this comment.
In general, it's fine to use "Pathlib", but here, it's a little odd, as the previous argument is also a path, but of str type. Please use the same type (it's probably simpler to use str).
(same comment for run_milling_tasks_fibsemos)
| stage_version = md_calib.get("version", None) | ||
|
|
||
| if stage_version == "tfs_3": | ||
| return create_fibsemos_tfs_microscope() |
There was a problem hiding this comment.
Shouldn't the tfs version do the same? At least, it would feel more logical that it's also possible to override the default path.
|
|
||
| # Define the default settings for fibsemOS | ||
| self.default.add_section("fibsemos") | ||
| self.default.set("fibsemos", "config_path", "microscope-configuration.yaml") |
There was a problem hiding this comment.
That doesn't look like a good default value. Because that means that by default it will try to a load a microscope-configuration.yaml file from the current working directory. That can be pretty much anywhere. I think it'd be best to keep the same as currently: by default, it passes None, which is converted into the full path to the standard fibsemOS folder.
In practice, I think you could use "" (empty string) as default, and have _run_automated_milling convert the empty string to None.
| # Define the default settings for milling | ||
| self.default.add_section("milling") | ||
| self.default.set("milling", "rate", "1e-8") | ||
| self.default.set("milling", "dwell_time", "1e-6") |
There was a problem hiding this comment.
I don't think it's a good idea to store the "general parameters" for the milling tasks in a completely different place (and format) than the rest of the milling parameters. I'd suggest the following instead:
- have a way to override the default
milling_tasks.yaml: when loading the file (by callingload_milling_tasks()), first look if it is present in~/.config/odemis/, and then fallback toDEFAULT_MILLING_TASKS_PATH. Then pass this path toload_milling_tasks(). - Extend
milling_tasks.yamlformat to support having general parameters. One possibility is to have a special "task" with the key "*" (and no "name"). Whenload_milling_tasks()finds such task (if it exists), it copies themillingparameters to all the other tasks (and remove that special task). So the file would look like this:
'*': # general parameters, applied to all the tasks
milling:
rate: 1.0e-9
dwell_time: 3.e-6
'Rough Milling 01':
name: 'Rough Milling 01'
milling:
current: 1.0e-9
voltage: 30000.0
field_of_view: 8e-05
mode: 'Serial'
channel: 'ion'
:If you are worried that the fibsemos config entry is a little lonely in this file, you could move it to the GeneralConfig, but don't think that's necessary. We will probably soon have to store some other settings about milling here.
2 participants
milling.configfile to define the milling rate, dwell time, and the path to the fibsemOS configuration file.