Skip to content

ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI #1923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oliwenmandiamond wants to merge 7 commits into
base: main
Choose a base branch
from
Open

ElectronAnalyser no longer depends directly on sequence file path to work with BlueAPI #1923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
59d30c8
ElectronAnalyser no longer depends directly on sequence file path to …
oliwenmandiamond Feb 17, 2026
7d4d359
Merge branch 'main' into analyser_works_with_blueapi
oliwenmandiamond Feb 17, 2026
3075079
Improve type checking
oliwenmandiamond Feb 17, 2026
9c2887e
Move PsuMode to B07 shared, improve type checking
oliwenmandiamond Feb 17, 2026
d87a608
Merge branch 'main' into analyser_works_with_blueapi
oliwenmandiamond Feb 19, 2026
7a97a23
Moved some pytest fixtures to where they are used
oliwenmandiamond Feb 19, 2026
de1c820
Merge branch 'main' into analyser_works_with_blueapi
oliwenmandiamond Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/dodal/beamlines/b07.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
B07SampleManipulator52B,
Grating,
LensMode,
PsuMode,
)
from dodal.devices.beamlines.b07_shared import PsuMode
from dodal.devices.electron_analyser.base import EnergySource
from dodal.devices.electron_analyser.specs import SpecsDetector
from dodal.devices.motors import XYZPolarStage
Expand Down
2 changes: 1 addition & 1 deletion src/dodal/beamlines/b07_1.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
from dodal.beamlines.b07_shared import devices as b07_shared_devices
from dodal.common.beamlines.beamline_utils import set_beamline as set_utils_beamline
from dodal.device_manager import DeviceManager
from dodal.devices.beamlines.b07 import PsuMode
from dodal.devices.beamlines.b07_1 import (
ChannelCutMonochromator,
Grating,
LensMode,
)
from dodal.devices.beamlines.b07_shared import PsuMode
from dodal.devices.electron_analyser.base import EnergySource
from dodal.devices.electron_analyser.specs import SpecsDetector
from dodal.devices.motors import XYZPolarAzimuthStage
Expand Down
4 changes: 2 additions & 2 deletions src/dodal/devices/beamlines/b07/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from .b07_motors import B07SampleManipulator52B
from .enums import Grating, LensMode, PsuMode
from .enums import Grating, LensMode

__all__ = ["B07SampleManipulator52B", "Grating", "LensMode", "PsuMode"]
__all__ = ["B07SampleManipulator52B", "Grating", "LensMode"]
12 changes: 0 additions & 12 deletions src/dodal/devices/beamlines/b07/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,3 @@ class LensMode(SupersetEnum):
# option if disconnected. Once it is connected, "Not connected" is replaced with the
# options above. This is also why this must be a SupersetEnum.
NOT_CONNECTED = "Not connected"


class PsuMode(SupersetEnum):
V3500 = "3.5kV"
V1500 = "1.5kV"
V400 = "400V"
V100 = "100V"
V10 = "10V"
# This is connected to the device separately and will only have "Not connected" as
# option if disconnected. Once it is connected, "Not connected" is replaced with the
# options above. This is also why this must be a SupersetEnum.
NOT_CONNECTED = "Not connected"
3 changes: 3 additions & 0 deletions src/dodal/devices/beamlines/b07_shared/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
from .enums import PsuMode

__all__ = ["PsuMode"]
13 changes: 13 additions & 0 deletions src/dodal/devices/beamlines/b07_shared/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from ophyd_async.core import SupersetEnum


class PsuMode(SupersetEnum):
V3500 = "3.5kV"
V1500 = "1.5kV"
V400 = "400V"
V100 = "100V"
V10 = "10V"
# This is connected to the device separately and will only have "Not connected" as
# option if disconnected. Once it is connected, "Not connected" is replaced with the
# options above. This is also why this must be a SupersetEnum.
NOT_CONNECTED = "Not connected"
44 changes: 15 additions & 29 deletions src/dodal/devices/electron_analyser/base/base_detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
TriggerInfo,
)

from dodal.common.data_util import load_json_file_to_class
from dodal.devices.electron_analyser.base.base_controller import (
ElectronAnalyserController,
)
Expand All @@ -20,9 +19,7 @@
)
from dodal.devices.electron_analyser.base.base_region import (
GenericRegion,
GenericSequence,
TAbstractBaseRegion,
TAbstractBaseSequence,
)


Expand Down Expand Up @@ -111,6 +108,9 @@ async def trigger(self) -> None:
await super().trigger()


# Used in sm-bluesky, but will hopefully be removed along with
# ElectronAnalyserRegionDetector in future. Blocked by:
# https://github.com/bluesky/bluesky/pull/1978
GenericElectronAnalyserRegionDetector = ElectronAnalyserRegionDetector[
GenericAnalyserDriverIO, GenericRegion
]
Expand All @@ -123,7 +123,7 @@ async def trigger(self) -> None:
class ElectronAnalyserDetector(
BaseElectronAnalyserDetector[TAbstractAnalyserDriverIO, TAbstractBaseRegion],
Stageable,
Generic[TAbstractBaseSequence, TAbstractAnalyserDriverIO, TAbstractBaseRegion],
Generic[TAbstractAnalyserDriverIO, TAbstractBaseRegion],
):
"""Electron analyser detector with the additional functionality to load a sequence
file and create a list of temporary ElectronAnalyserRegionDetector objects. These
Expand All @@ -132,13 +132,13 @@ class ElectronAnalyserDetector(

def __init__(
self,
sequence_class: type[TAbstractBaseSequence],
controller: ElectronAnalyserController[
TAbstractAnalyserDriverIO, TAbstractBaseRegion
],
name: str = "",
):
self._sequence_class = sequence_class
# Save on device so connect works and names it as child
self.driver = controller.driver
Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a driver that is created and passed in a child class, here in parent class it is taken out from the controller and saved as a class object - wouldn't it be clearer to make it in a child class directly when driver is created?

Copy link
Contributor Author

@oliwenmandiamond oliwenmandiamond Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is then in charge of making sure the driver and detector connect and parent/name relationship is correct rather than the API. It's much better to make sure the API does it as all classes will need to do this, otherwise we copy and paste this line x number times for number of implementations which is unnecessary.

Copy link
Contributor

@Villtord Villtord Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably won't need to copy paste anything just add "self.driver" instead of making internal "driver" in a child class

Copy link
Contributor Author

@oliwenmandiamond oliwenmandiamond Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this is that if we type anything as using the GenericElectronAnalyser for tests or plans or other devices i.e it doesn't care about if it Specs, Mbs, VGScienta etc. then it loses access to the driver because it lives now on the implementation and not on the base. We need it to be done here.

Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like your use case for this is only relevant for tests, you probably could in analyser-related plan specify a parameter as GenericElectronAnalyser in a plan but you anyway want to operate using verbs so Movable, Readable or StandardDetector class (when it's there) would suffice? Isn't it the whole point of this change to get rid of generic analyser, am I missing smth?

Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or are you planning to call bps.move(analyser.driver) in a plan?

Copy link
Contributor Author

@oliwenmandiamond oliwenmandiamond Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the Bluesky change goes through with allowing devices have multiple configuration, then we can remove method create_region_detector_listand class GenericElectronAnalyserRegionDetector and GenericBaseElectronAnalyserDetector and just have only GenericElectronAnalyserDetector. We have to live with this complexity until this is done. This change is focusing on working with what what we got and letting it work with BlueAPI by removing file path dependency. The generic classes currently are only used in tests yes. They are a very convenient way to keep full typing for classes. Otherwise to have full typing I would have to do something like below

ElectronAnalyserDetector[AbastractRegion, AbstractDriverIO[AbstractRegion]]

and that isn't including the PsuMode, PassEnergy and LensMode types.

I think the reason they aren't used in the plans themselves is because they are invariant so has a couple of issues, but again this isn't related to this change.

Plans can still have full typing of the device and would need it to access child devices e.g

E.g

def my_plan(my_device: Movable):
     yield from mv(my_device.child_device1, 2) #invalid type checking
     yield from mv(my_device.child_device2, 5) #invalid type checking

If you give the correct type for the plan static type checking won't complain.

If you dislike the generics classes because they are only used in tests and not plans at the moment we can address this in a new issue/PR.

Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather dislike that inverse logic of having driver in base class via controller in child class that actually creates that driver -> which seems to be justified only for having GenericElectronAnalyser device having access to driver which is used only in tests and plan tests.

super().__init__(controller, name)

@AsyncStatus.wrap
Expand All @@ -160,38 +160,24 @@ async def unstage(self) -> None:
"""Disarm the detector."""
await self._controller.disarm()

def load_sequence(self, filename: str) -> TAbstractBaseSequence:
"""Load the sequence data from a provided json file into a sequence class.
Args:
filename (str): Path to the sequence file containing the region data.
Returns:
Pydantic model representing the sequence file.
"""
return load_json_file_to_class(self._sequence_class, filename)

def create_region_detector_list(
self, filename: str, enabled_only=True
self, regions: list[TAbstractBaseRegion]
) -> list[
ElectronAnalyserRegionDetector[TAbstractAnalyserDriverIO, TAbstractBaseRegion]
]:
"""Create a list of detectors equal to the number of regions in a sequence file.
Each detector is responsible for setting up a specific region.
"""This method can hopefully be dropped when this is merged and released.
https://github.com/bluesky/bluesky/pull/1978.
Create a list of detectors equal to the number of regions. Each detector is
responsible for setting up a specific region.
Args:
filename (str): Path to the sequence file containing the region data.
enabled_only (bool, optional): If true, only include the region if enabled
is True.
regions: The list of regions to give to each region detector.
Returns:
List of ElectronAnalyserRegionDetector, equal to the number of regions in
the sequence file.
the sequence file.
"""
seq = self.load_sequence(filename)
regions: list[TAbstractBaseRegion] = (
seq.get_enabled_regions() if enabled_only else seq.regions
)
return [
ElectronAnalyserRegionDetector[
TAbstractAnalyserDriverIO, TAbstractBaseRegion
Expand All @@ -201,7 +187,7 @@ def create_region_detector_list(


GenericElectronAnalyserDetector = ElectronAnalyserDetector[
Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need this class here? it seems to be used only in tests

Copy link
Contributor Author

@oliwenmandiamond oliwenmandiamond Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this will be used in plan repo too.

Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which plan repo? sm-bluesky?

Copy link
Contributor Author

@oliwenmandiamond oliwenmandiamond Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

https://github.com/search?q=repo%3ADiamondLightSource%2Fsm-bluesky%20GenericElectronAnalyserDetector&type=code

Copy link
Contributor

@Villtord Villtord Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm again in tests...:)

GenericSequence, GenericAnalyserDriverIO, GenericRegion
GenericAnalyserDriverIO, GenericRegion
]
TElectronAnalyserDetector = TypeVar(
"TElectronAnalyserDetector",
Expand Down
16 changes: 4 additions & 12 deletions src/dodal/devices/electron_analyser/specs/specs_detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,13 @@
from dodal.devices.electron_analyser.base.base_region import TLensMode, TPsuMode
from dodal.devices.electron_analyser.base.energy_sources import AbstractEnergySource
from dodal.devices.electron_analyser.specs.specs_driver_io import SpecsAnalyserDriverIO
from dodal.devices.electron_analyser.specs.specs_region import (
SpecsRegion,
SpecsSequence,
)
from dodal.devices.electron_analyser.specs.specs_region import SpecsRegion
from dodal.devices.fast_shutter import FastShutter
from dodal.devices.selectable_source import SourceSelector


class SpecsDetector(
ElectronAnalyserDetector[
SpecsSequence[TLensMode, TPsuMode],
SpecsAnalyserDriverIO[TLensMode, TPsuMode],
SpecsRegion[TLensMode, TPsuMode],
],
Expand All @@ -33,15 +29,11 @@ def __init__(
source_selector: SourceSelector | None = None,
name: str = "",
):
# Save to class so takes part with connect()
self.driver = SpecsAnalyserDriverIO[TLensMode, TPsuMode](
driver = SpecsAnalyserDriverIO[TLensMode, TPsuMode](
prefix, lens_mode_type, psu_mode_type
)

controller = ElectronAnalyserController[
SpecsAnalyserDriverIO[TLensMode, TPsuMode], SpecsRegion[TLensMode, TPsuMode]
](self.driver, energy_source, shutter, source_selector)

sequence_class = SpecsSequence[lens_mode_type, psu_mode_type]
](driver, energy_source, shutter, source_selector)

super().__init__(sequence_class, controller, name)
super().__init__(controller, name)
13 changes: 3 additions & 10 deletions src/dodal/devices/electron_analyser/vgscienta/vgscienta_detector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,13 @@
from dodal.devices.electron_analyser.vgscienta.vgscienta_region import (
TPassEnergyEnum,
VGScientaRegion,
VGScientaSequence,
)
from dodal.devices.fast_shutter import FastShutter
from dodal.devices.selectable_source import SourceSelector


class VGScientaDetector(
ElectronAnalyserDetector[
VGScientaSequence[TLensMode, TPsuMode, TPassEnergyEnum],
VGScientaAnalyserDriverIO[TLensMode, TPsuMode, TPassEnergyEnum],
VGScientaRegion[TLensMode, TPassEnergyEnum],
],
Expand All @@ -37,17 +35,12 @@ def __init__(
source_selector: SourceSelector | None = None,
name: str = "",
):
# Save to class so takes part with connect()
self.driver = VGScientaAnalyserDriverIO[TLensMode, TPsuMode, TPassEnergyEnum](
driver = VGScientaAnalyserDriverIO[TLensMode, TPsuMode, TPassEnergyEnum](
prefix, lens_mode_type, psu_mode_type, pass_energy_type
)

controller = ElectronAnalyserController[
VGScientaAnalyserDriverIO[TLensMode, TPsuMode, TPassEnergyEnum],
VGScientaRegion[TLensMode, TPassEnergyEnum],
](self.driver, energy_source, shutter, source_selector)
](driver, energy_source, shutter, source_selector)

sequence_class = VGScientaSequence[
lens_mode_type, psu_mode_type, pass_energy_type
]
super().__init__(sequence_class, controller, name)
super().__init__(controller, name)
6 changes: 0 additions & 6 deletions src/dodal/testing/electron_analyser/__init__.py
View file
Open in desktop

This file was deleted.

57 changes: 0 additions & 57 deletions src/dodal/testing/electron_analyser/device_factory.py
View file
Open in desktop

This file was deleted.

20 changes: 20 additions & 0 deletions tests/devices/electron_analyser/base/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import pytest

from dodal.devices.beamlines import b07, b07_shared, i09
from dodal.devices.electron_analyser.base import GenericElectronAnalyserDetector
from dodal.devices.electron_analyser.specs import SpecsDetector
from dodal.devices.electron_analyser.vgscienta import VGScientaDetector


@pytest.fixture(params=["ew4000", "b07b_specs150"])
def sim_detector(
request: pytest.FixtureRequest,
ew4000: VGScientaDetector[i09.LensMode, i09.PsuMode, i09.PassEnergy],
b07b_specs150: SpecsDetector[b07.LensMode, b07_shared.PsuMode],
) -> GenericElectronAnalyserDetector:
detectors = [ew4000, b07b_specs150]
for detector in detectors:
if detector.name == request.param:
return detector

raise ValueError(f"Detector with name '{request.param}' not found")
Loading