[S30-130][fix] Store e-beam current and voltage as metadata#3490
Open
pieleric wants to merge 2 commits into
Open
Conversation
pieleric
changed the title
📝 WalkthroughWalkthroughMetadataUpdater.observeEbeam now subscribes to the e-beam's probeCurrent and accelVoltage and propagates those values to affected components (rotation still forwarded only for multibeam). StreamController.display_metadata shows acceleration voltage and emission current only for EM or FIB acquisition types and reads MD_BEAM* keys. New integration tests (EbeamMDUpdaterTest) exercise the simulated SEM flow and assert metadata propagation. Sequence Diagram(s)sequenceDiagram
participant EBeamScanner
participant MetadataUpdater
participant DetectorComponent
participant StreamController
EBeamScanner->>MetadataUpdater: VA update (probeCurrent / accelVoltage)
MetadataUpdater->>DetectorComponent: set MD_BEAM_CURRENT / MD_BEAM_VOLTAGE
StreamController->>DetectorComponent: read metadata for display (if acquisitionType EM/FIB)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (2)
src/odemis/odemisd/test/mdupdater_test.py (1)
36-37: 💤 Low valueConsider using pathlib.Path for file path construction.
Lines 36-37 construct file paths using string concatenation and
os.path.dirname. As per coding guidelines, file paths should usepathlib.Pathunless interfacing with existing code that requires strings. Iftesting.start_backendaccepts path-like objects, refactor to usepathlib.Pathfor better cross-platform compatibility and readability.♻️ Example refactor using pathlib.Path
+from pathlib import Path + import odemis from odemis import model from odemis.driver import static from odemis.model import Microscope from odemis.odemisd.mdupdater import MetadataUpdater from odemis.util import mock, testing -CONFIG_PATH = os.path.dirname(odemis.__file__) + "/../../install/linux/usr/share/odemis/" -SPARC2_CONFIG = CONFIG_PATH + "sim/sparc2-sim-scanner.odm.yaml" +CONFIG_PATH = Path(odemis.__file__).parent / "../../install/linux/usr/share/odemis/" +SPARC2_CONFIG = str(CONFIG_PATH / "sim/sparc2-sim-scanner.odm.yaml")Note:
str()wrapper retained only iftesting.start_backendrequires a string path.🤖 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/odemisd/test/mdupdater_test.py` around lines 36 - 37, Replace string-based path construction with pathlib.Path: create CONFIG_PATH as Path(odemis.__file__).resolve().parent.joinpath("..","..","install","linux","usr","share","odemis") (or equivalent) and set SPARC2_CONFIG = CONFIG_PATH / "sim" / "sparc2-sim-scanner.odm.yaml"; when calling testing.start_backend, pass the Path object directly if it accepts path-like objects, otherwise wrap with str() (references: CONFIG_PATH, SPARC2_CONFIG, odemis, testing.start_backend).Source: Coding guidelines
src/odemis/gui/cont/stream.py (1)
287-291: ⚡ Quick winUse consistent metadata keys: check and retrieve the same key name.
Lines 287-288 check for
MD_BEAM_VOLTAGEbut retrievemd[MD_EBEAM_VOLTAGE]. Lines 290-291 do the same forMD_BEAM_CURRENTvsMD_EBEAM_CURRENT. While these are aliases pointing to the same string, the inconsistency is confusing. SinceMetadataUpdater.observeEbeam(in mdupdater.py) now populatesMD_BEAM_VOLTAGEandMD_BEAM_CURRENT, the GUI should consistently check for and retrieve those same keys.♻️ Proposed fix for consistency
if self.stream.acquisitionType.value in (model.MD_AT_EM, model.MD_AT_FIB): if model.MD_BEAM_VOLTAGE in md: - self.add_metadata("Acceleration voltage", md[model.MD_EBEAM_VOLTAGE], 'V') + self.add_metadata("Acceleration voltage", md[model.MD_BEAM_VOLTAGE], 'V') if model.MD_BEAM_CURRENT in md: - self.add_metadata("Emission current", md[model.MD_EBEAM_CURRENT], 'A') + self.add_metadata("Emission current", md[model.MD_BEAM_CURRENT], 'A')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/cont/stream.py` around lines 287 - 291, The GUI checks for model.MD_BEAM_VOLTAGE and model.MD_BEAM_CURRENT but retrieves md[model.MD_EBEAM_VOLTAGE] and md[model.MD_EBEAM_CURRENT], which is inconsistent with MetadataUpdater.observeEbeam; update the retrieval to use the same keys you check for (md[model.MD_BEAM_VOLTAGE] and md[model.MD_BEAM_CURRENT]) so add_metadata calls use the identical metadata keys, leaving add_metadata and MetadataUpdater.observeEbeam unchanged.
🤖 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/odemisd/mdupdater.py`:
- Around line 622-629: The subscription to ebeam.rotation should be guarded so
we don't AttributeError when the VA is missing: before creating updateRotation
and calling ebeam.rotation.subscribe, check that the ebeam actually exposes the
rotation VA (e.g., use ebeam.hasVA('rotation') or equivalent) and only then
define updateRotation, subscribe with init=True, append to self._onTerminate
(unsubscribe tuple) and set observed=True; if the check fails, skip the
subscription logic for that ebeam.
In `@src/odemis/odemisd/test/mdupdater_test.py`:
- Around line 134-136: The test assumes probeCurrent.choices supports set
subtraction; update the assignment to coerce choices into a set before
subtracting the current value: convert self.ebeam.probeCurrent.choices into
set(...) and then subtract {self.ebeam.probeCurrent.value}, then pick an element
and assign it back to self.ebeam.probeCurrent.value; change the expression that
computes new_current in the mdupdater_test (the lines using
probeCurrent.choices, probeCurrent.value, and new_current) to use
set(self.ebeam.probeCurrent.choices) to avoid TypeError for non-set iterables.
---
Nitpick comments:
In `@src/odemis/gui/cont/stream.py`:
- Around line 287-291: The GUI checks for model.MD_BEAM_VOLTAGE and
model.MD_BEAM_CURRENT but retrieves md[model.MD_EBEAM_VOLTAGE] and
md[model.MD_EBEAM_CURRENT], which is inconsistent with
MetadataUpdater.observeEbeam; update the retrieval to use the same keys you
check for (md[model.MD_BEAM_VOLTAGE] and md[model.MD_BEAM_CURRENT]) so
add_metadata calls use the identical metadata keys, leaving add_metadata and
MetadataUpdater.observeEbeam unchanged.
In `@src/odemis/odemisd/test/mdupdater_test.py`:
- Around line 36-37: Replace string-based path construction with pathlib.Path:
create CONFIG_PATH as
Path(odemis.__file__).resolve().parent.joinpath("..","..","install","linux","usr","share","odemis")
(or equivalent) and set SPARC2_CONFIG = CONFIG_PATH / "sim" /
"sparc2-sim-scanner.odm.yaml"; when calling testing.start_backend, pass the Path
object directly if it accepts path-like objects, otherwise wrap with str()
(references: CONFIG_PATH, SPARC2_CONFIG, odemis, testing.start_backend).
🪄 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: d4fdd0c4-a88f-4aca-8b09-2d356139b7a9
📒 Files selected for processing (3)
src/odemis/gui/cont/stream.pysrc/odemis/odemisd/mdupdater.pysrc/odemis/odemisd/test/mdupdater_test.py
There was a problem hiding this comment.
Pull request overview
This PR extends MetadataUpdater’s e-beam observer so that e-beam probe current and acceleration voltage are propagated as MD_BEAM_CURRENT and MD_BEAM_VOLTAGE to all affected components (and still propagates rotation for multibeam). It also updates the GUI metadata display logic and adds an integration test using the simulated SPARC2 scanner config.
Changes:
- Propagate
probeCurrent→MD_BEAM_CURRENTandaccelVoltage→MD_BEAM_VOLTAGEfor components affected by the e-beam. - Adjust GUI metadata display to avoid showing beam metadata on non-EM/FIB streams.
- Add an integration test validating detector metadata updates when e-beam parameters change (simulated backend).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/odemis/odemisd/mdupdater.py | Enhances e-beam metadata propagation to include beam current/voltage (plus multibeam rotation). |
| src/odemis/odemisd/test/mdupdater_test.py | Adds an integration test verifying metadata propagation for beam current/voltage via the simulated SPARC2 backend. |
| src/odemis/gui/cont/stream.py | Updates which streams display beam voltage/current metadata in the GUI. |
pieleric
force-pushed
the
feat-odemisd-md-updater-also-takes-care-of-beam_current-and-beam_voltage
branch
from
b6f8e11 to
c3513fb
Compare
…LTAGE So far, we assumed that if the e-beam driver changes the current of voltage, it would update the metadata. However, there are two issues: * if it's a CompositedScanner, the component in charge of the column is not the same as the one acquiring the data, and so it cannot update the metadata. * The metadata would be only on the e-beam data. However, on the SPARC, in case of CL, the optical image (spectrometer) also depends on the current & voltage, so it's worthy to store the metadata on it too.
…FIB streams We used to show the metadata for all the streams, but until now only SEM & FIB streams had this information (in some rare cases). Now, SEM streams have this info almost all the time, and CL streams too, on the SPARC. However, on the CL streams, it's always the same as the concurrent SEM stream, so no need to display it too.
pieleric
force-pushed
the
feat-odemisd-md-updater-also-takes-care-of-beam_current-and-beam_voltage
branch
from
c3513fb to
bf4a4b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/odemisd/test/mdupdater_test.py`:
- Around line 97-137: Add missing type hints and docstrings for the listed test
methods and helper: annotate setUpClass(cls) -> None, tearDownClass(cls) ->
None, test_ebeam_beam_params_on_detector(self) -> None, and
fake_get_component(name: str) -> ComponentType (or appropriate model.Component
return type), and add short reStructuredText docstrings for setUpClass,
tearDownClass, and fake_get_component describing their purpose; ensure
docstrings follow repo style and import any typing names needed (e.g., from
typing import Any or the model Component type) so the test file compiles with
type checking.
- Around line 116-117: The test sets cls.ebeam.affects to include the real
target "se-detector" but only asserts metadata on the fake CCD (cls.ccd); update
the test(s) around the cls.ebeam.affects usage (including the block covering
lines ~139-173) to also locate the actual "se-detector" target (e.g., by
querying the registry/manager by name or by resolving the path used in the code
under test) and assert the same propagated metadata values on that object as you
do for cls.ccd, ensuring you reference cls.ebeam.affects, cls.ccd.name and the
literal "se-detector" to find and validate the real detector.
🪄 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: 8eaea8ad-909e-4a63-8ab8-651525bb2a4d
📒 Files selected for processing (3)
src/odemis/gui/cont/stream.pysrc/odemis/odemisd/mdupdater.pysrc/odemis/odemisd/test/mdupdater_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/odemis/gui/cont/stream.py
- src/odemis/odemisd/mdupdater.py
Comment on lines
+97
to
+137
| def setUpClass(cls): | ||
| # Create a minimal sparc2 microscope | ||
| cls.mic = Microscope("Fake SPARC2", "sparc2") | ||
|
|
||
| # Create a SimSEM with an e-beam scanner and an se-detector child | ||
| cls.sem = simsem.SimSEM( | ||
| "SEM", | ||
| "sem", | ||
| children={ | ||
| "scanner": {"name": "e-beam scanner", "role": "e-beam"}, | ||
| "detector0": {"name": "se-detector", "role": "se-detector"}, | ||
| } | ||
| ) | ||
| cls.ebeam = next(c for c in cls.sem.children.value if c.role == "e-beam") | ||
|
|
||
| # Create a fake CCD that will be affected by the e-beam | ||
| img = model.DataArray(numpy.empty((512, 768), dtype=numpy.uint16)) | ||
| cls.ccd = mock.FakeCCD(img) | ||
|
|
||
| cls.ebeam.affects.value = [cls.ccd.name, "se-detector"] | ||
|
|
||
| # Mock model.getComponent() so MetadataUpdater can resolve component names | ||
| all_comps = list(cls.sem.children.value) + [cls.sem, cls.ccd] | ||
|
|
||
| def fake_get_component(name): | ||
| for c in all_comps: | ||
| if c.name == name: | ||
| return c | ||
| raise LookupError(f"no component {name}") | ||
|
|
||
| cls._patch_get_component = unittest.mock.patch.object(model, "getComponent", fake_get_component) | ||
| cls._patch_get_component.start() | ||
|
|
||
| cls.mdup = MetadataUpdater("MDUpdater", cls.mic) | ||
| cls.mic.alive.value = set(cls.sem.children.value) | {cls.ccd} | ||
|
|
||
| @classmethod | ||
| def tearDownClass(cls): | ||
| cls.mdup.terminate() | ||
| cls.sem.terminate() | ||
| cls._patch_get_component.stop() |
There was a problem hiding this comment.
Add required type hints and function docstrings on new test methods/helpers.
setUpClass, tearDownClass, test_ebeam_beam_params_on_detector, and fake_get_component are missing required type hints, and setUpClass / tearDownClass / fake_get_component are missing function docstrings mandated by repo rules.
Suggested patch
class EbeamMDUpdaterTest(unittest.TestCase):
@@
`@classmethod`
- def setUpClass(cls):
+ def setUpClass(cls) -> None:
+ """
+ Build a simulated microscope setup and start metadata updater wiring.
+ """
@@
- def fake_get_component(name):
+ def fake_get_component(name: str):
+ """
+ Resolve a component by its name from the local simulated component list.
+ """
for c in all_comps:
if c.name == name:
return c
raise LookupError(f"no component {name}")
@@
`@classmethod`
- def tearDownClass(cls):
+ def tearDownClass(cls) -> None:
+ """
+ Tear down updater, simulated hardware, and patches created for this test class.
+ """
cls.mdup.terminate()
cls.sem.terminate()
cls._patch_get_component.stop()
@@
- def test_ebeam_beam_params_on_detector(self):
+ def test_ebeam_beam_params_on_detector(self) -> None:As per coding guidelines: **/*.py: Always use type hints for function parameters and return types in Python code; include docstrings for all functions and classes (reStructuredText style).
Also applies to: 139-139
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 119-119: Consider [*list(cls.sem.children.value), cls.sem, cls.ccd] instead of concatenation
Replace with [*list(cls.sem.children.value), cls.sem, cls.ccd]
(RUF005)
🤖 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/odemisd/test/mdupdater_test.py` around lines 97 - 137, Add missing
type hints and docstrings for the listed test methods and helper: annotate
setUpClass(cls) -> None, tearDownClass(cls) -> None,
test_ebeam_beam_params_on_detector(self) -> None, and fake_get_component(name:
str) -> ComponentType (or appropriate model.Component return type), and add
short reStructuredText docstrings for setUpClass, tearDownClass, and
fake_get_component describing their purpose; ensure docstrings follow repo style
and import any typing names needed (e.g., from typing import Any or the model
Component type) so the test file compiles with type checking.
Source: Coding guidelines
Comment on lines
+116
to
+117
| cls.ebeam.affects.value = [cls.ccd.name, "se-detector"] | ||
|
|
There was a problem hiding this comment.
Validate metadata propagation on the actual se-detector too, not only the fake CCD.
The test configures two affected targets (cls.ccd.name and "se-detector"), but assertions only check cls.ccd. This misses regressions where propagation fails for the real detector path.
Suggested patch
cls.ebeam = next(c for c in cls.sem.children.value if c.role == "e-beam")
+ cls.se_detector = next(c for c in cls.sem.children.value if c.role == "se-detector")
@@
md = self.ccd.getMetadata()
+ md_se = self.se_detector.getMetadata()
@@
self.assertAlmostEqual(md[model.MD_BEAM_CURRENT], self.ebeam.probeCurrent.value)
self.assertAlmostEqual(md[model.MD_BEAM_VOLTAGE], self.ebeam.accelVoltage.value)
+ self.assertAlmostEqual(md_se[model.MD_BEAM_CURRENT], self.ebeam.probeCurrent.value)
+ self.assertAlmostEqual(md_se[model.MD_BEAM_VOLTAGE], self.ebeam.accelVoltage.value)
@@
md = self.ccd.getMetadata()
+ md_se = self.se_detector.getMetadata()
self.assertAlmostEqual(md[model.MD_BEAM_CURRENT], new_current)
+ self.assertAlmostEqual(md_se[model.MD_BEAM_CURRENT], new_current)
@@
md = self.ccd.getMetadata()
+ md_se = self.se_detector.getMetadata()
self.assertAlmostEqual(md[model.MD_BEAM_VOLTAGE], new_voltage)
+ self.assertAlmostEqual(md_se[model.MD_BEAM_VOLTAGE], new_voltage)Also applies to: 139-173
🤖 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/odemisd/test/mdupdater_test.py` around lines 116 - 117, The test
sets cls.ebeam.affects to include the real target "se-detector" but only asserts
metadata on the fake CCD (cls.ccd); update the test(s) around the
cls.ebeam.affects usage (including the block covering lines ~139-173) to also
locate the actual "se-detector" target (e.g., by querying the registry/manager
by name or by resolving the path used in the code under test) and assert the
same propagated metadata values on that object as you do for cls.ccd, ensuring
you reference cls.ebeam.affects, cls.ccd.name and the literal "se-detector" to
find and validate the real detector.
nandishjpatel
approved these changes
Jun 16, 2026
tmoerkerken
approved these changes
Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The metadata of the e-beam when controlled via API (eg, current and accel voltage) is never stored on the data.
=> extend the mdupdater to listen to the standard VAs of the e-beam and update the meteadata of all affected detectors
This ensures that it’s be displayed in the GUI, and in the exported image databar.