[MSD-302][feat] milling patterns takes spot size input#3495
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds an explicit FIB beam spot_size (diameter) milling parameter and uses it to expand the displayed milling rectangle overlays so the on-screen boxes circumscribe the beam path (accounting for beam tails).
Changes:
- Add
spot_sizetoMillingSettings(serialization + YAML defaults) and expose it in the milling task UI. - Expand GUI milling rectangle overlays by
spot_sizein both width/height when drawing patterns. - Fix panel config handling to avoid
KeyErrorwhen removing thelabelkey.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/odemis/gui/cont/milling.py | Pass spot_size into overlay generation and expand rectangle dimensions accordingly; wire UI updates to include the new parameter. |
| src/odemis/gui/comp/milling.py | Add UI configuration for spot_size and make label removal safe via pop(). |
| src/odemis/acq/milling/tasks.py | Add spot_size VA to MillingSettings and include it in (de)serialization. |
| src/odemis/acq/milling/milling_tasks.yaml | Add default spot_size values to the built-in task presets and update comments. |
| @@ -101,12 +101,19 @@ | |||
| ref_img: model.DataArray, | |||
| pattern: RectanglePatternParameters, | |||
| colour: str = "#FFFF00", | |||
| name: str = None) -> EditableShape: | |||
| """Convert a rectangle pattern to a shape""" | |||
| rect = RectangleOverlay(cnvs=canvas, colour = colour, show_selection_points = False) | |||
| name: str = None, | |||
| spot_size: float = 0.0) -> EditableShape: | |||
| Updates milling time and availability of the mill button when there's an update on the patterns | ||
| """ | ||
|
|
||
| logging.warning(f"Pattern updated: {dat}") |
📝 WalkthroughWalkthroughThe pull request adds Sequence Diagram(s)sequenceDiagram
participant MillingTaskPanel
participant MillingController
participant rectangle_pattern_to_shape
participant ProjectState
MillingTaskPanel->>MillingController: update spot_size control values
MillingController->>rectangle_pattern_to_shape: render with spot_size=task.milling.spot_size.value
rectangle_pattern_to_shape->>rectangle_pattern_to_shape: expand width and height by spot_size
MillingController->>ProjectState: save_project(main)
MillingController->>MillingController: redraw milling overlays
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)src/odemis/gui/cont/milling.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.21][ERROR]: unable to find a config; path src/odemis/gui/comp/milling.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.14][ERROR]: unable to find a config; path src/odemis/acq/milling/tasks.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.25][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/odemis/gui/cont/milling.py (1)
523-523: ⚡ Quick winConsider using
logging.debuginstead oflogging.warning.The
_on_patternshandler is called on every parameter change (width, height, depth, spacing, current, align, mode, spot_size) due to VA subscriptions. Usinglogging.warninghere could spam the logs with routine updates. Consider usinglogging.debugunless pattern updates indicate an actual warning condition.♻️ Proposed change
- logging.warning(f"Pattern updated: {dat}") + logging.debug(f"Pattern updated: {dat}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/milling.py` at line 523, Change the logging level in the _on_patterns handler from logging.warning to logging.debug for the "Pattern updated" message. This log statement is called frequently due to VA subscriptions monitoring routine parameter changes (width, height, depth, spacing, current, align, mode, spot_size), and using warning level causes unnecessary log spam. Since these are routine updates and not actual warning conditions, using debug level is more appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/acq/milling/tasks.py`:
- Around line 36-47: The __init__ method of the MillingSettings class is missing
a docstring. Add a reStructuredText style docstring to the __init__ method that
documents all parameters including current, voltage, field_of_view, mode,
channel, align, and spot_size. Each parameter should be documented with its
type, unit (where applicable), range/choices, and a brief description of what it
represents. Use the :param type name: description format for consistency with
project guidelines.
In `@src/odemis/gui/cont/milling.py`:
- Around line 104-105: The `name` parameter in the method signature has an
incorrect type hint. The parameter is declared as `str` but it has a default
value of `None` and can be `None` based on usage at line 117 and line 420.
Update the type hint for the `name` parameter to accept both string and None
values by changing it to `str | None` or using `Optional[str]` (if using older
Python typing style).
- Around line 107-109: Update the docstring for the function that converts a
rectangle pattern to a shape to follow reStructuredText format. Expand the
current single-line summary to include proper parameter documentation using
:param: directives for all parameters (such as canvas and colour), and add a
:return: directive describing what the function returns. Ensure the docstring
maintains the summary line about rectangle pattern conversion and beam spot
expansion while adding the required reStructuredText sections for complete
parameter and return value documentation.
---
Nitpick comments:
In `@src/odemis/gui/cont/milling.py`:
- Line 523: Change the logging level in the _on_patterns handler from
logging.warning to logging.debug for the "Pattern updated" message. This log
statement is called frequently due to VA subscriptions monitoring routine
parameter changes (width, height, depth, spacing, current, align, mode,
spot_size), and using warning level causes unnecessary log spam. Since these are
routine updates and not actual warning conditions, using debug level is more
appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 240e9dcb-a6e1-43f7-bf87-554dcc3c53fd
📒 Files selected for processing (4)
src/odemis/acq/milling/milling_tasks.yamlsrc/odemis/acq/milling/tasks.pysrc/odemis/gui/comp/milling.pysrc/odemis/gui/cont/milling.py
| class MillingSettings: | ||
| """Represents milling settings for a single milling task""" | ||
|
|
||
| def __init__(self, current: float, voltage: float, field_of_view: float, mode: str = "Serial", channel: str = "ion", align: bool = True): | ||
| def __init__(self, current: float, voltage: float, field_of_view: float, mode: str = "Serial", | ||
| channel: str = "ion", align: bool = True, spot_size: float = 50.0e-09): | ||
| self.current = model.FloatContinuous(current, unit="A", range=(20e-12, 120e-9)) | ||
| self.voltage = model.FloatContinuous(voltage, unit="V", range=(0, 30e3)) | ||
| self.field_of_view = model.FloatContinuous(field_of_view, unit="m", range=(50e-06, 960e-06)) | ||
| self.mode = model.StringEnumerated(mode, choices={"Serial", "Parallel"}) | ||
| self.channel = model.StringEnumerated(channel, choices={"ion"}) | ||
| self.align = model.BooleanVA(align) # align at the milling current | ||
| self.align = model.BooleanVA(align) # align at the milling current | ||
| self.spot_size = model.FloatContinuous(spot_size, unit="m", range=(5.0e-09, 1.0e-06)) # FIB beam spot size (diameter) |
There was a problem hiding this comment.
Add docstring for MillingSettings class and __init__ method.
The MillingSettings class and its __init__ method lack docstrings. As per coding guidelines, all classes and functions should have docstrings following the reStructuredText style.
📝 Proposed docstring
class MillingSettings:
- """Represents milling settings for a single milling task"""
+ """
+ Represents milling settings for a single milling task.
+
+ Encapsulates all parameters required for FIB milling operations,
+ including beam current, voltage, field of view, and spot size.
+ """
- def __init__(self, current: float, voltage: float, field_of_view: float, mode: str = "Serial",
- channel: str = "ion", align: bool = True, spot_size: float = 50.0e-09):
+ def __init__(
+ self,
+ current: float,
+ voltage: float,
+ field_of_view: float,
+ mode: str = "Serial",
+ channel: str = "ion",
+ align: bool = True,
+ spot_size: float = 50.0e-09
+ ) -> None:
+ """
+ Initialize milling settings.
+
+ :param current: Beam current in amperes
+ :param voltage: Beam voltage in volts
+ :param field_of_view: Field of view in meters
+ :param mode: Milling mode, either Serial or Parallel
+ :param channel: Channel to use for milling
+ :param align: Whether to align at the milling current
+ :param spot_size: FIB beam spot size diameter in meters
+ """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/acq/milling/tasks.py` around lines 36 - 47, The __init__ method of
the MillingSettings class is missing a docstring. Add a reStructuredText style
docstring to the __init__ method that documents all parameters including
current, voltage, field_of_view, mode, channel, align, and spot_size. Each
parameter should be documented with its type, unit (where applicable),
range/choices, and a brief description of what it represents. Use the :param
type name: description format for consistency with project guidelines.
Source: Coding guidelines
| name: str = None, | ||
| spot_size: float = 0.0) -> EditableShape: |
There was a problem hiding this comment.
Fix type hint for name parameter.
The name parameter can be None (as checked on line 117 and passed as None on line 420), but the type hint is str. It should be str | None or Optional[str].
🔧 Proposed fix
def rectangle_pattern_to_shape(canvas,
ref_img: model.DataArray,
pattern: RectanglePatternParameters,
colour: str = "`#FFFF00`",
- name: str = None,
+ name: str | None = None,
spot_size: float = 0.0) -> EditableShape:📝 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.
| name: str = None, | |
| spot_size: float = 0.0) -> EditableShape: | |
| def rectangle_pattern_to_shape(canvas, | |
| ref_img: model.DataArray, | |
| pattern: RectanglePatternParameters, | |
| colour: str = "`#FFFF00`", | |
| name: str | None = None, | |
| spot_size: float = 0.0) -> EditableShape: |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 104-104: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/cont/milling.py` around lines 104 - 105, The `name` parameter
in the method signature has an incorrect type hint. The parameter is declared as
`str` but it has a default value of `None` and can be `None` based on usage at
line 117 and line 420. Update the type hint for the `name` parameter to accept
both string and None values by changing it to `str | None` or using
`Optional[str]` (if using older Python typing style).
Source: Linters/SAST tools
K4rishma
force-pushed
the
spot-size-millling-patterns
branch
from
da12606 to
27bd2fb
Compare
| # milling params: current, voltage, field of view, mode | ||
| milling_parameters = ["current", "align", "mode"] | ||
| pattern_parameters = ["width", "height", "depth", "spacing"] | ||
| # milling params: current, voltage, field of view, mode, spot_size |
There was a problem hiding this comment.
Why is this comment here and how does it relate to the code? Does "voltage" correspond to "align" and "field of view" to "mode"?
There was a problem hiding this comment.
I just added the spot size to preciously mentioned parameters. I do not know if voltage affects alignment, fov the mode. But according to the comment, it might be the case.
I can see that the comment is confusing nonetheless, I have decided to remove it as it does not bring any insights.
K4rishma
force-pushed
the
spot-size-millling-patterns
branch
from
27bd2fb to
ce5decb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/odemis/acq/milling/tasks.py (2)
49-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the serialization helpers.
to_dict()andfrom_dict()are part of the config contract, but both changed methods are still undocumented. As per coding guidelines, all functions should have docstrings. Based on learnings, keep them plain text only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/milling/tasks.py` around lines 49 - 66, Document the serialization helpers in MillingSettings by adding plain-text docstrings to to_dict() and from_dict(); these methods define the config contract, so briefly describe that to_dict() serializes the current settings to a dictionary and from_dict() reconstructs a MillingSettings instance from a dictionary, including the default fallback values used for optional fields.Sources: Coding guidelines, Learnings
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing return annotation on
__init__.This changed signature still violates the Python typing rule because
__init__has no explicit-> None.Suggested fix
- def __init__(self, current: float, voltage: float, field_of_view: float, mode: str = "Serial", - channel: str = "ion", align: bool = True, spot_size: float = 5.0e-09): + def __init__(self, current: float, voltage: float, field_of_view: float, mode: str = "Serial", + channel: str = "ion", align: bool = True, spot_size: float = 5.0e-09) -> None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/acq/milling/tasks.py` around lines 39 - 40, The __init__ method in the task class is missing an explicit return annotation and should be updated to declare that it returns None. Add the missing `-> None` to the constructor signature for the `__init__` method so the typing is correct and consistent with Python conventions.Source: Coding guidelines
src/odemis/gui/cont/milling.py (1)
106-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this docstring in plain text.
This file already has a repo-specific preference against
:param:/:return:markup in docstrings, so this should be rewritten as plain text while keeping the spot-size explanation. Based on learnings, docstrings in this file should use plain text only, not RST directives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/milling.py` around lines 106 - 115, The docstring in the rectangle-to-shape conversion routine should be rewritten from RST-style parameter markup to plain text only. Update the docstring on the shape conversion helper in milling.py so it keeps the spot-size explanation and the description of the canvas, reference image, pattern, colour, and name, but removes all :param: directives and any similar markup.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/odemis/acq/milling/tasks.py`:
- Around line 49-66: Document the serialization helpers in MillingSettings by
adding plain-text docstrings to to_dict() and from_dict(); these methods define
the config contract, so briefly describe that to_dict() serializes the current
settings to a dictionary and from_dict() reconstructs a MillingSettings instance
from a dictionary, including the default fallback values used for optional
fields.
- Around line 39-40: The __init__ method in the task class is missing an
explicit return annotation and should be updated to declare that it returns
None. Add the missing `-> None` to the constructor signature for the `__init__`
method so the typing is correct and consistent with Python conventions.
In `@src/odemis/gui/cont/milling.py`:
- Around line 106-115: The docstring in the rectangle-to-shape conversion
routine should be rewritten from RST-style parameter markup to plain text only.
Update the docstring on the shape conversion helper in milling.py so it keeps
the spot-size explanation and the description of the canvas, reference image,
pattern, colour, and name, but removes all :param: directives and any similar
markup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c8dd351-0dc9-4725-ae6d-5a6af5176965
📒 Files selected for processing (4)
src/odemis/acq/milling/milling_tasks.yamlsrc/odemis/acq/milling/tasks.pysrc/odemis/gui/comp/milling.pysrc/odemis/gui/cont/milling.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/odemis/gui/comp/milling.py
|
Reason for converting to draft -
|
|
@K4rishma I agree with you that it should go back to the specification level. I've replied in the Jira task. |
The “hole size” parameter of the Essence preset (in XML) currently contains the diameter of the orifice that will be milled out by FIB. The milling boxes displayed in Odemis should circumscribe the beam path to account for so-called beam tails.