[MSD-39][feature] Adjust the Odemis FIBSEM milling tab to Tescan settings#3305
[MSD-39][feature] Adjust the Odemis FIBSEM milling tab to Tescan settings#3305ilyushkin wants to merge 11 commits intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds milling configuration and parameter propagation across acquisition and GUI: new MillingConfig (rate, dwell_time, fibsemos config_path) and get_milling_conf(); MillingSettings gains rate and dwell_time; load_milling_tasks accepts and merges general_params into task milling settings; CryoFeature accepts general_params and GUI passes milling_conf values when creating features; config_path is threaded through microscope factories, FibsemOS managers, and run_milling_tasks_fibsemos; fibsemos path now supports in-memory reference-image retrieval and cropping; Automated milling flow no longer performs the separate reference-image alignment step; StaticFIBStream added for FIB acquisition routing. Sequence Diagram(s)mermaid 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)
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.
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.
src/odemis/acq/milling/fibsemos.py
Outdated
| 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)
| @@ -107,7 +108,7 @@ def create_fibsemos_microscope() -> 'OdemisThermoMicroscope | OdemisTescanMicros | |||
| 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.
| if fibsemos.FIBSEMOS_INSTALLED: | ||
| self._mill_future = run_milling_tasks_fibsemos(tasks=tasks, config_path=self.milling_conf.config_path) | ||
| else: | ||
| self._mill_future = millmng.run_milling_tasks(tasks=tasks, fib_stream=self._tab.fib_stream) |
There was a problem hiding this comment.
So we still allow the "old" flow. Are we going to maintain/test that code path in the future?
ilyushkin
force-pushed
the
msd-39-adjust-odemis-to-tescan-settings
branch
from
4ae1749 to
d54c333
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/odemis/acq/milling/test/fibsemos_millmng_test.py (1)
26-103:⚠️ Potential issue | 🟡 MinorAdd return type hints and a docstring for
setUpClass.The methods in this test class are missing return type hints. Per the coding guidelines, all functions must include return type hints. Additionally,
setUpClasslacks a docstring.✏️ Example update
`@classmethod` - def setUpClass(cls): + def setUpClass(cls) -> None: + """Initialize the fibsemOS test fixture.""" @@ - def test_estimate_total_milling_time(self): + def test_estimate_total_milling_time(self) -> None: @@ - def test_fibsemos_milling_manager(self): + def test_fibsemos_milling_manager(self) -> None: @@ - def test_cancel_milling(self): + def test_cancel_milling(self) -> None:src/odemis/acq/milling/tasks.py (1)
39-70: 🛠️ Refactor suggestion | 🟠 MajorAdd reST docstrings and explicit
-> Nonereturn type forMillingSettings.__init__.The
__init__method is missing the explicit-> Nonereturn type annotation, and__init__,to_dict, andfrom_dictall lack reST docstrings. Per project guidelines, all functions and classes must include docstrings and proper type hints.Example update
- def __init__(self, current: float, voltage: float, field_of_view: float, rate: float, dwell_time: float, mode: str = "Serial", channel: str = "ion", align: bool = True): + def __init__(self, current: float, voltage: float, field_of_view: float, rate: float, dwell_time: float, mode: str = "Serial", channel: str = "ion", align: bool = True) -> None: + """Initialize milling settings. + + :param current: Beam current. + :param voltage: Beam voltage. + :param field_of_view: Field of view. + :param rate: Milling rate. + :param dwell_time: Dwell time. + :param mode: Patterning mode. + :param channel: Beam channel. + :param align: Align at milling current. + """Also add docstrings to
to_dict()andfrom_dict().src/odemis/acq/milling/fibsemos.py (1)
126-168:⚠️ Potential issue | 🟠 MajorFix config-path typing and the return annotation.
Line 126:
config_path: Path = Noneshould use explicit optional syntaxPath | None = Nonefor Python 3.10+ compliance.Line 157:
Union['OdemisThermoMicroscope | OdemisTescanMicroscope']incorrectly mixes Union with pipe syntax. Should be eitherUnion['OdemisThermoMicroscope', 'OdemisTescanMicroscope']or the modern form'OdemisThermoMicroscope | OdemisTescanMicroscope'without Union.✏️ Suggested update
-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) -> Union['OdemisThermoMicroscope | OdemisTescanMicroscope']: +def create_fibsemos_microscope(config_path: Path | None = None) -> OdemisThermoMicroscope | OdemisTescanMicroscope:src/odemis/acq/milling/millmng.py (1)
299-319:⚠️ Potential issue | 🟠 MajorAdd explicit Optional typing and document
config_pathparameter in docstrings.
UsePath | None = None(Python 3.10+) and add:param config_path:to docstrings for bothAutomatedMillingManager.__init__andrun_automated_milling.✏️ Suggested update
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: Path | None = None ): + """ + Initialize the AutomatedMillingManager. + + :param future: Future for tracking milling progress + :param features: List of features to mill + :param stage: Stage actuator + :param sem_stream: SEM stream for imaging + :param fib_stream: FIB stream for milling + :param task_list: List of milling workflow tasks + :param config_path: Optional path to configuration file + """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: Path | None = None ) -> Future: """ Automatically mill and image a list of features. + :param config_path: Optional path to configuration file :return: ProgressiveFuture """Also applies to: 514-537
🤖 Fix all issues with AI agents
In `@src/odemis/acq/milling/fibsemos.py`:
- Around line 63-75: Update the docstrings for the helper functions
_get_reference_image and _format_preset to reStructuredText style by adding
:param, :return, and :raises entries; specifically for _get_reference_image
include ":param feature: CryoFeature the feature to read the reference image
from", ":return: model.DataArray the in-memory reference image", and ":raises
ValueError: if feature.reference_image is None", and similarly document
_format_preset’s parameters, return value and any exceptions it can raise (use
the actual parameter names and types from the function signature so the
docstrings match the code).
- Around line 288-295: The manager currently always calls
create_fibsemos_microscope(config_path) only once and ignores later differing
config_path values; to fix, store the provided config_path on the instance
(e.g., self._config_path) in __init__ and update any code paths that obtain the
microscope (or add a helper like _ensure_microscope(config_path)) to compare the
incoming config_path against self._config_path and recreate self.microscope via
create_fibsemos_microscope(config_path) when they differ, ensuring you also
update self._config_path and preserve thread-safety using the existing
self._lock around recreation.
In `@src/odemis/acq/milling/test/fibsemos_reference_image_test.py`:
- Around line 19-22: The test method test_raises_if_missing_in_memory uses
setattr(feature, "reference_image", None) which is less clear and triggers B010;
replace that call with a direct attribute assignment feature.reference_image =
None to match the style used elsewhere (e.g., earlier in the test) and avoid the
flake8-bugbear warning; keep the rest of the test and the CryoFeature
instantiation unchanged.
- Around line 1-28: Add reST docstrings and explicit "-> None" return type hints
to the test class and its test methods: update the class
TestResolveFeatureReferenceImage and both methods
test_returns_in_memory_reference_image and test_raises_if_missing_in_memory to
include a one- or two-line reST-style docstring describing purpose, and annotate
each method signature with "-> None". Ensure imports/types remain valid and that
docstrings follow the project's reST format.
In `@src/odemis/gui/conf/file.py`:
- Around line 578-617: Update MillingConfig to add type hints and reST
docstrings: annotate __init__(self) -> None and add a short reST docstring
describing construction and default sections; annotate property getters
rate(self) -> float, dwell_time(self) -> float, config_path(self) -> str and add
reST ":return:" descriptions for each; annotate setters rate(self, value: float)
-> None, dwell_time(self, value: float) -> None, config_path(self, config_path:
str) -> None and add reST ":param:" descriptions for their parameters; keep
existing logic and names (MillingConfig, __init__, rate, dwell_time,
config_path) unchanged.
In `@src/odemis/gui/cont/milling.py`:
- Around line 481-485: The call to run_milling_tasks_fibsemos is missing the
required feature: CryoFeature argument; update the branch where
fibsemos.FIBSEMOS_INSTALLED is true (the assignment to self._mill_future) to
pass feature=self._tab_data.main.currentFeature.value along with tasks and
config_path so the call becomes run_milling_tasks_fibsemos(tasks=tasks,
config_path=self.milling_conf.config_path,
feature=self._tab_data.main.currentFeature.value).
🧹 Nitpick comments (1)
src/odemis/gui/cont/stream_bar.py (1)
2286-2288: Misleading warning message after type check change.The isinstance check now filters for
StaticFIBStreamspecifically, but the warning message still says "Unexpected non static stream". This is misleading because aStaticSEMStreamwould trigger this warning even though it is a static stream—just not a FIB stream.✏️ Suggested fix
if not isinstance(sc.stream, StaticFIBStream): - logging.warning("Unexpected non static stream: %s", sc.stream) + logging.warning("Unexpected non-FIB stream: %s", sc.stream) continue
| def _get_reference_image(feature: CryoFeature) -> model.DataArray: | ||
| """Get the in-memory reference image for a feature or raise.""" | ||
|
|
||
| if feature.reference_image is None: | ||
| logging.error( | ||
| "Missing reference image for feature '%s' (path=%s). " | ||
| "This feature was likely loaded from disk without hydrating reference_image.", | ||
| feature.name.value, | ||
| getattr(feature, "path", None), | ||
| ) | ||
| raise ValueError("Missing feature.reference_image.") | ||
| return feature.reference_image | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the file and check its existence
find . -name "fibsemos.py" -type fRepository: delmic/odemis
Length of output: 93
🏁 Script executed:
# Read the specific lines mentioned in the review
if [ -f "src/odemis/acq/milling/fibsemos.py" ]; then
echo "=== Lines 60-80 (around _get_reference_image) ==="
sed -n '60,80p' src/odemis/acq/milling/fibsemos.py
echo ""
echo "=== Lines 215-250 (around _format_preset) ==="
sed -n '215,250p' src/odemis/acq/milling/fibsemos.py
fiRepository: delmic/odemis
Length of output: 1866
Update docstrings to reST style with :param, :return, and :raises tags.
The helper functions _get_reference_image (line 63) and _format_preset (line 217) have docstrings that do not follow the reStructuredText style guide required by project guidelines.
✏️ Example update
def _get_reference_image(feature: CryoFeature) -> model.DataArray:
- """Get the in-memory reference image for a feature or raise."""
+ """Get the in-memory reference image for a feature.
+
+ :param feature: Feature holding the reference image.
+ :return: In-memory reference image.
+ :raises ValueError: If reference_image is missing.
+ """
def _format_preset(voltage: float, current: float) -> str:
- """
- Format voltage (V) and current (A) into a string like:
- '30 keV; 150 pA', scaling units automatically.
- """
+ """Format voltage and current into a preset string.
+
+ :param voltage: Beam voltage in V.
+ :param current: Beam current in A.
+ :return: Formatted preset string (e.g., '30 keV; 150 pA').
+ """🧰 Tools
🪛 Ruff (0.14.14)
[warning] 73-73: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@src/odemis/acq/milling/fibsemos.py` around lines 63 - 75, Update the
docstrings for the helper functions _get_reference_image and _format_preset to
reStructuredText style by adding :param, :return, and :raises entries;
specifically for _get_reference_image include ":param feature: CryoFeature the
feature to read the reference image from", ":return: model.DataArray the
in-memory reference image", and ":raises ValueError: if feature.reference_image
is None", and similarly document _format_preset’s parameters, return value and
any exceptions it can raise (use the actual parameter names and types from the
function signature so the docstrings match the code).
| def __init__(self, config_path: Optional[str] = None): | ||
| """Initialize the manager and establish the fibsemOS microscope connection.""" | ||
| # create microscope connection | ||
| self.microscope = create_fibsemos_microscope() | ||
| self.microscope = create_fibsemos_microscope(config_path) | ||
| self._lock = threading.Lock() | ||
| self._active = False | ||
| self._cancel_requested = False | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/odemis/acq/milling && wc -l fibsemos.pyRepository: delmic/odemis
Length of output: 72
🏁 Script executed:
cd src/odemis/acq/milling && sed -n '280,310p' fibsemos.pyRepository: delmic/odemis
Length of output: 1322
🏁 Script executed:
cd src/odemis/acq/milling && sed -n '380,410p' fibsemos.pyRepository: delmic/odemis
Length of output: 784
🏁 Script executed:
cd src/odemis/acq/milling && rg "_persistent_millmng" fibsemos.py -B 2 -A 2Repository: delmic/odemis
Length of output: 694
🏁 Script executed:
cd src/odemis/acq/milling && head -50 fibsemos.py | grep -E "^import|^from"Repository: delmic/odemis
Length of output: 517
🏁 Script executed:
cd src/odemis/acq/milling && rg "from pathlib|import Path" fibsemos.pyRepository: delmic/odemis
Length of output: 81
config_path changes are ignored after the first manager creation.
If config_path can vary per run, the persistent manager will keep using the first path. Store the path on the manager and recreate when it differs.
🛠️ One way to guard against stale config_path
- def __init__(self, config_path: Optional[str] = None):
+ def __init__(self, config_path: Path | None = None):
"""Initialize the manager and establish the fibsemOS microscope connection."""
+ self.config_path = config_path
# create microscope connection
self.microscope = create_fibsemos_microscope(config_path)
@@
-def run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], feature: CryoFeature, path: Optional[str] = None, config_path: Optional[str] = None) -> futures.Future:
+def run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], feature: CryoFeature, path: Optional[str] = None, config_path: Path | None = None) -> futures.Future:
@@
- if _persistent_millmng is None:
- _persistent_millmng = FibsemOSMillingTaskManager(config_path)
+ if (
+ _persistent_millmng is None
+ or (config_path is not None and _persistent_millmng.config_path != config_path)
+ ):
+ _persistent_millmng = FibsemOSMillingTaskManager(config_path)🤖 Prompt for AI Agents
In `@src/odemis/acq/milling/fibsemos.py` around lines 288 - 295, The manager
currently always calls create_fibsemos_microscope(config_path) only once and
ignores later differing config_path values; to fix, store the provided
config_path on the instance (e.g., self._config_path) in __init__ and update any
code paths that obtain the microscope (or add a helper like
_ensure_microscope(config_path)) to compare the incoming config_path against
self._config_path and recreate self.microscope via
create_fibsemos_microscope(config_path) when they differ, ensuring you also
update self._config_path and preserve thread-safety using the existing
self._lock around recreation.
| import unittest | ||
|
|
||
| import numpy | ||
|
|
||
| from odemis import model | ||
| from odemis.acq.feature import CryoFeature | ||
| from odemis.acq.milling.fibsemos import _get_reference_image | ||
|
|
||
|
|
||
| class TestResolveFeatureReferenceImage(unittest.TestCase): | ||
| def test_returns_in_memory_reference_image(self): | ||
| feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) | ||
| da = model.DataArray(numpy.zeros((10, 12), dtype=numpy.uint16), metadata={model.MD_DIMS: "YX"}) | ||
| feature.reference_image = da | ||
|
|
||
| out = _get_reference_image(feature) | ||
| self.assertIs(out, da) | ||
|
|
||
| def test_raises_if_missing_in_memory(self): | ||
| feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) | ||
| setattr(feature, "reference_image", None) | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| _get_reference_image(feature) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and show its content
if [ -f "src/odemis/acq/milling/test/fibsemos_reference_image_test.py" ]; then
echo "File exists."
cat -n "src/odemis/acq/milling/test/fibsemos_reference_image_test.py"
else
echo "File not found. Checking directory structure..."
find . -name "*fibsemos*" -type f 2>/dev/null | head -20
fiRepository: delmic/odemis
Length of output: 1156
🏁 Script executed:
# Also check if there are other test files in the same directory to see the style conventions
ls -la "src/odemis/acq/milling/test/" 2>/dev/null | head -20Repository: delmic/odemis
Length of output: 592
🏁 Script executed:
# Check other test files to see the convention for docstrings and type hints
echo "=== fibsemos_test.py (first 50 lines) ==="
head -50 "src/odemis/acq/milling/test/fibsemos_test.py"
echo -e "\n=== millmng_test.py (first 50 lines) ==="
head -50 "src/odemis/acq/milling/test/millmng_test.py"Repository: delmic/odemis
Length of output: 2985
🏁 Script executed:
# Check if test classes and methods in existing test files have docstrings and type hints
echo "=== fibsemos_test.py - looking for test methods ==="
grep -A 3 "def test_" "src/odemis/acq/milling/test/fibsemos_test.py" | head -40
echo -e "\n=== millmng_test.py - looking for test methods ==="
grep -A 3 "def test_" "src/odemis/acq/milling/test/millmng_test.py" | head -40
echo -e "\n=== patterns_test.py - looking for test methods ==="
grep -A 3 "def test_" "src/odemis/acq/milling/test/patterns_test.py" | head -40Repository: delmic/odemis
Length of output: 3326
Add reST docstrings and -> None return type hints to the class and test methods.
The test class and its methods must follow the project's mandatory docstring and typing conventions for all Python files.
✏️ Example update
-class TestResolveFeatureReferenceImage(unittest.TestCase):
- def test_returns_in_memory_reference_image(self):
+class TestResolveFeatureReferenceImage(unittest.TestCase):
+ """Tests for resolving in-memory reference images."""
+
+ def test_returns_in_memory_reference_image(self) -> None:
+ """Return the in-memory reference image when present."""
@@
- def test_raises_if_missing_in_memory(self):
+ def test_raises_if_missing_in_memory(self) -> None:
+ """Raise when reference_image is missing."""🧰 Tools
🪛 Ruff (0.14.14)
[warning] 21-21: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🤖 Prompt for AI Agents
In `@src/odemis/acq/milling/test/fibsemos_reference_image_test.py` around lines 1
- 28, Add reST docstrings and explicit "-> None" return type hints to the test
class and its test methods: update the class TestResolveFeatureReferenceImage
and both methods test_returns_in_memory_reference_image and
test_raises_if_missing_in_memory to include a one- or two-line reST-style
docstring describing purpose, and annotate each method signature with "-> None".
Ensure imports/types remain valid and that docstrings follow the project's reST
format.
| def test_raises_if_missing_in_memory(self): | ||
| feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) | ||
| setattr(feature, "reference_image", None) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/odemis/acq/milling/test/fibsemos_reference_image_test.py | head -30Repository: delmic/odemis
Length of output: 1143
🏁 Script executed:
fd "fibsemos_reference_image_test.py"Repository: delmic/odemis
Length of output: 117
Use direct assignment instead of setattr().
Clearer and more Pythonic. Direct assignment is already used elsewhere in the test (line 14) and avoids the B010 flake8-bugbear warning.
🔧 Small cleanup
- setattr(feature, "reference_image", None)
+ feature.reference_image = None🧰 Tools
🪛 Ruff (0.14.14)
[warning] 21-21: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🤖 Prompt for AI Agents
In `@src/odemis/acq/milling/test/fibsemos_reference_image_test.py` around lines 19
- 22, The test method test_raises_if_missing_in_memory uses setattr(feature,
"reference_image", None) which is less clear and triggers B010; replace that
call with a direct attribute assignment feature.reference_image = None to match
the style used elsewhere (e.g., earlier in the test) and avoid the
flake8-bugbear warning; keep the rest of the test and the CryoFeature
instantiation unchanged.
| class MillingConfig(Config): | ||
| """ FIB milling configuration values """ | ||
|
|
||
| file_name = "milling.config" | ||
|
|
||
| def __init__(self): | ||
| super(MillingConfig, self).__init__() | ||
|
|
||
| # 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") | ||
|
|
||
| # Define the default settings for fibsemOS | ||
| self.default.add_section("fibsemos") | ||
| self.default.set("fibsemos", "config_path", "microscope-configuration.yaml") | ||
|
|
||
| @property | ||
| def rate(self): | ||
| return float(self.get("milling", "rate")) | ||
|
|
||
| @rate.setter | ||
| def rate(self, value): | ||
| self.set("milling", "rate", value) | ||
|
|
||
| @property | ||
| def dwell_time(self): | ||
| return float(self.get("milling", "dwell_time")) | ||
|
|
||
| @dwell_time.setter | ||
| def dwell_time(self, value): | ||
| self.set("milling", "dwell_time", value) | ||
|
|
||
| @property | ||
| def config_path(self): | ||
| return self.get("fibsemos", "config_path") | ||
|
|
||
| @config_path.setter | ||
| def config_path(self, config_path): | ||
| self.set("fibsemos", "config_path", config_path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
head -n 700 src/odemis/gui/conf/file.py | tail -n +570 | head -n 60Repository: delmic/odemis
Length of output: 1474
🏁 Script executed:
# Also check the Config parent class definition in the same file
rg -n "^class Config" src/odemis/gui/conf/file.pyRepository: delmic/odemis
Length of output: 92
🏁 Script executed:
# Check if there are existing type hints or docstrings in similar Config subclasses
rg -A 5 "class.*Config.*:" src/odemis/gui/conf/file.py | head -n 50Repository: delmic/odemis
Length of output: 898
🏁 Script executed:
# Check GeneralConfig and AcquisitionConfig for type hints and docstrings
sed -n '36,150p' src/odemis/gui/conf/file.py | head -n 100Repository: delmic/odemis
Length of output: 3936
🏁 Script executed:
# Check CalibrationConfig as well
rg -A 30 "class CalibrationConfig" src/odemis/gui/conf/file.py | head -n 40Repository: delmic/odemis
Length of output: 1444
Add type hints and reST docstrings to MillingConfig methods.
The __init__, property getters, and property setters lack type hints and docstrings as required by coding guidelines for Python files.
✏️ Example update
def __init__(self):
+ """Initialize milling configuration defaults."""
+ def __init__(self) -> None:
super(MillingConfig, self).__init__()
`@property`
def rate(self):
+ """Return the milling rate."""
+ def rate(self) -> float:
return float(self.get("milling", "rate"))
`@rate.setter`
def rate(self, value):
+ """Set the milling rate."""
+ def rate(self, value: float) -> None:
self.set("milling", "rate", value)
`@property`
def dwell_time(self):
+ """Return the milling dwell time."""
+ def dwell_time(self) -> float:
return float(self.get("milling", "dwell_time"))
`@dwell_time.setter`
def dwell_time(self, value):
+ """Set the milling dwell time."""
+ def dwell_time(self, value: float) -> None:
self.set("milling", "dwell_time", value)
`@property`
def config_path(self):
+ """Return the fibsemOS config path."""
+ def config_path(self) -> str:
return self.get("fibsemos", "config_path")
`@config_path.setter`
def config_path(self, config_path):
+ """Set the fibsemOS config path."""
+ def config_path(self, config_path: str) -> None:
self.set("fibsemos", "config_path", config_path)📝 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.
| class MillingConfig(Config): | |
| """ FIB milling configuration values """ | |
| file_name = "milling.config" | |
| def __init__(self): | |
| super(MillingConfig, self).__init__() | |
| # 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") | |
| # Define the default settings for fibsemOS | |
| self.default.add_section("fibsemos") | |
| self.default.set("fibsemos", "config_path", "microscope-configuration.yaml") | |
| @property | |
| def rate(self): | |
| return float(self.get("milling", "rate")) | |
| @rate.setter | |
| def rate(self, value): | |
| self.set("milling", "rate", value) | |
| @property | |
| def dwell_time(self): | |
| return float(self.get("milling", "dwell_time")) | |
| @dwell_time.setter | |
| def dwell_time(self, value): | |
| self.set("milling", "dwell_time", value) | |
| @property | |
| def config_path(self): | |
| return self.get("fibsemos", "config_path") | |
| @config_path.setter | |
| def config_path(self, config_path): | |
| self.set("fibsemos", "config_path", config_path) | |
| class MillingConfig(Config): | |
| """FIB milling configuration values""" | |
| file_name = "milling.config" | |
| def __init__(self) -> None: | |
| """Initialize milling configuration defaults.""" | |
| super(MillingConfig, self).__init__() | |
| # 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") | |
| # Define the default settings for fibsemOS | |
| self.default.add_section("fibsemos") | |
| self.default.set("fibsemos", "config_path", "microscope-configuration.yaml") | |
| `@property` | |
| def rate(self) -> float: | |
| """Return the milling rate.""" | |
| return float(self.get("milling", "rate")) | |
| `@rate.setter` | |
| def rate(self, value: float) -> None: | |
| """Set the milling rate.""" | |
| self.set("milling", "rate", value) | |
| `@property` | |
| def dwell_time(self) -> float: | |
| """Return the milling dwell time.""" | |
| return float(self.get("milling", "dwell_time")) | |
| `@dwell_time.setter` | |
| def dwell_time(self, value: float) -> None: | |
| """Set the milling dwell time.""" | |
| self.set("milling", "dwell_time", value) | |
| `@property` | |
| def config_path(self) -> str: | |
| """Return the fibsemOS config path.""" | |
| return self.get("fibsemos", "config_path") | |
| `@config_path.setter` | |
| def config_path(self, config_path: str) -> None: | |
| """Set the fibsemOS config path.""" | |
| self.set("fibsemos", "config_path", config_path) |
🤖 Prompt for AI Agents
In `@src/odemis/gui/conf/file.py` around lines 578 - 617, Update MillingConfig to
add type hints and reST docstrings: annotate __init__(self) -> None and add a
short reST docstring describing construction and default sections; annotate
property getters rate(self) -> float, dwell_time(self) -> float,
config_path(self) -> str and add reST ":return:" descriptions for each; annotate
setters rate(self, value: float) -> None, dwell_time(self, value: float) ->
None, config_path(self, config_path: str) -> None and add reST ":param:"
descriptions for their parameters; keep existing logic and names (MillingConfig,
__init__, rate, dwell_time, config_path) unchanged.
|
|
||
| if fibsemos.FIBSEMOS_INSTALLED: | ||
| self._mill_future = run_milling_tasks_fibsemos(tasks=tasks, config_path=self.milling_conf.config_path) | ||
| else: | ||
| self._mill_future = millmng.run_milling_tasks(tasks=tasks, fib_stream=self._tab.fib_stream) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the function signature in fibsemos.py
rg -A 15 "def run_milling_tasks_fibsemos" src/odemis/acq/milling/fibsemos.pyRepository: delmic/odemis
Length of output: 590
🏁 Script executed:
# Now let's check the call site in milling.py and verify the context
rg -B 10 -A 5 "run_milling_tasks_fibsemos" src/odemis/gui/cont/milling.pyRepository: delmic/odemis
Length of output: 1406
🏁 Script executed:
# Let's examine the class structure to understand what self._tab_data contains
rg -B 30 "def __init__" src/odemis/gui/cont/milling.py | head -60Repository: delmic/odemis
Length of output: 2216
🏁 Script executed:
# Look at the MillingTaskController __init__ more carefully
sed -n '100,200p' src/odemis/gui/cont/milling.py | head -80Repository: delmic/odemis
Length of output: 3185
🏁 Script executed:
# Search for currentFeature in the codebase
rg "currentFeature" src/Repository: delmic/odemis
Length of output: 7666
🏁 Script executed:
# Final verification: check the usage of currentFeature in milling.py around the milling code
rg -B 5 -A 5 "currentFeature" src/odemis/gui/cont/milling.pyRepository: delmic/odemis
Length of output: 1983
Add missing feature argument to run_milling_tasks_fibsemos call.
The function signature requires a feature: CryoFeature parameter, but the call at lines 481-485 only passes tasks and config_path. Use self._tab_data.main.currentFeature.value to obtain the current feature.
Proposed fix
if fibsemos.FIBSEMOS_INSTALLED:
- self._mill_future = run_milling_tasks_fibsemos(tasks=tasks, config_path=self.milling_conf.config_path)
+ self._mill_future = run_milling_tasks_fibsemos(
+ tasks=tasks,
+ feature=self._tab_data.main.currentFeature.value,
+ config_path=self.milling_conf.config_path
+ )
else:
self._mill_future = millmng.run_milling_tasks(tasks=tasks, fib_stream=self._tab.fib_stream)🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/milling.py` around lines 481 - 485, The call to
run_milling_tasks_fibsemos is missing the required feature: CryoFeature
argument; update the branch where fibsemos.FIBSEMOS_INSTALLED is true (the
assignment to self._mill_future) to pass
feature=self._tab_data.main.currentFeature.value along with tasks and
config_path so the call becomes run_milling_tasks_fibsemos(tasks=tasks,
config_path=self.milling_conf.config_path,
feature=self._tab_data.main.currentFeature.value).
3 participants
milling.configfile to define the milling rate, dwell time, and the path to the fibsemOS configuration file.