Break up pds module into package, update slits device class, add slits to startup, update tests

[Jakub Wlodek (jwlodek)]

• Break up pds module into package, update slits device class, add slits to startup, update tests

[Copilot]

🟡 Changes recommended

The new hextools.photon_delivery_system package currently has blocking import/API mismatches (missing exports in __init__.py and a Slits constructor signature mismatch with tests) that will break imports and CI.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR refactors the photon delivery system (PDS) from a single module into a package, introduces a richer Slits device (gap/center derived signals), wires slits into the HEX collection profile baseline, and reorganizes the test suite to match the new package layout.

Changes:

• Split hextools.photon_delivery_system into a package with separate dclm, filters, shutter, and slits modules (plus packaged filters.yml).
• Added a derived-signal-based Slits implementation and included slit devices in the collection profile baseline metadata.
• Replaced the monolithic PDS test file with per-component tests (dclm, filters, shutter, slits).

File summaries

File	Description
tests/test_photon_delivery_system.py	Removes monolithic PDS tests in favor of per-module tests.
tests/photon_delivery_system/test_dclm.py	Adds DCLM/energy-change plan tests aligned to the new module structure.
tests/photon_delivery_system/test_filters.py	Adds filter parsing/motion tests using packaged YAML config.
tests/photon_delivery_system/test_shutter.py	Adds shutter open/close behavior tests for the new shutter module.
tests/photon_delivery_system/test_slits.py	Adds tests for slit gap/center derived signals and bps.mv integration.
src/hextools/utils.py	Adds helper to fetch typed objects from the IPython user namespace.
src/hextools/profiles/collection.py	Adds slit devices to startup and includes them in baseline metadata.
src/hextools/photon_delivery_system/init.py	Defines the new public package surface (__all__) for PDS devices/plans.
src/hextools/photon_delivery_system/dclm.py	Introduces DCLM device and updated change_energy plan (namespace lookup + shutter finalize).
src/hextools/photon_delivery_system/filters.py	Introduces Filter device classes and YAML-driven load_filters().
src/hextools/photon_delivery_system/filters.yml	Adds packaged filter configuration consumed by load_filters().
src/hextools/photon_delivery_system/shutter.py	Introduces shutter device module.
src/hextools/photon_delivery_system/slits.py	Introduces slit device with derived gap/center signals and multi-axis set.
src/hextools/photon_delivery_system.py	Removes the old monolithic PDS module.

Review details

• Files reviewed: 13/14 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

There is a confirmed import-time syntax/indentation error in filters.py and the new slits tests still use the old Slits(prefix, num) constructor signature, which will break the test suite.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

tests/photon_delivery_system/test_slits.py:78

• This test also uses the old Slits(prefix, num) constructor; Slits now expects (prefix, name=...). Update the instantiation to pass a string name and a prefix that matches the new slits PV layout.

    with init_devices(mock=True):
        slits = Slits("XF:TEST:", 1)
    for motor in (slits.inboard, slits.outboard, slits.bottom, slits.top):

• Files reviewed: 13/14 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are test-breaking and runtime-breaking issues (notably an IndentationError in filters.py and a Slits constructor mismatch in tests) that must be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tests/photon_delivery_system/test_slits.py:12

• Slits no longer accepts a numeric num argument (constructor is Slits(prefix: str, name: str = "")). This test still calls Slits("XF:TEST:", 1), which will raise TypeError and prevent the test suite from running.

    async with init_devices(mock=True):
        device = Slits("XF:TEST:", 1)

tests/photon_delivery_system/test_slits.py:78

• Same constructor mismatch as above: Slits("XF:TEST:", 1) will raise TypeError with the updated Slits API.

    with init_devices(mock=True):
        slits = Slits("XF:TEST:", 1)
    for motor in (slits.inboard, slits.outboard, slits.bottom, slits.top):

src/hextools/photon_delivery_system/filters.py:81

• _get_description is currently defined at module scope, which prematurely ends the Filter class block and leaves the subsequent indented @AsyncStatus.wrap/set method at an invalid indentation level. As-is, this file will fail to import with an IndentationError, and self._get_description won't exist on Filter.

def _get_description(self, in_pos: bool, motor_pos: float) -> str:
        """Get the description of the current filter setting based on motor position.

        Parameters
        ----------

• Files reviewed: 15/17 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new Slits API is not reflected in the newly added slits tests, and there are import hygiene issues in the collection profile that are likely to fail lint/type checks.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tests/photon_delivery_system/test_slits.py:12

• Slits was updated to take just (prefix, name=...), but the test still constructs it with the old (prefix, num) signature, which will raise TypeError and prevent these tests from running.

    async with init_devices(mock=True):
        device = Slits("XF:TEST:", 1)

tests/photon_delivery_system/test_slits.py:77

• This test still uses the legacy Slits(prefix, num) constructor; the new Slits class accepts only (prefix, name=...).

    with init_devices(mock=True):
        slits = Slits("XF:TEST:", 1)

src/hextools/profiles/collection.py:185

• Device names are used as data keys in Bluesky; using a hyphen can make downstream access awkward (e.g., attribute-style access and some serialization assumptions). Using underscores keeps naming consistent with the other devices in this profile.

        name="perkin-elmer",

• Files reviewed: 16/18 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The startup profile currently references undefined variables (wb_slits/pb_slits) and the new slits tests use an outdated Slits(...) constructor signature, which will cause immediate import/test failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tests/photon_delivery_system/test_slits.py:13

• The Slits constructor now takes (prefix: str, name: str = ""), but the test passes an integer as the second argument (Slits("XF:TEST:", 1)), which will be treated as name and can break device construction. Update the test to pass the fully-qualified slits PV prefix and an optional string name.

    async with init_devices(mock=True):
        device = Slits("XF:TEST:", 1)
    # Mirror each motor setpoint to its readback so moves complete in mock mode.

tests/photon_delivery_system/test_slits.py:78

• Same constructor mismatch as above: Slits("XF:TEST:", 1) passes an int where a name string is expected. Use the slits PV prefix and optional string name so bps.mv(slits, ...) drives the correct device.

    with init_devices(mock=True):
        slits = Slits("XF:TEST:", 1)
    for motor in (slits.inboard, slits.outboard, slits.bottom, slits.top):

src/hextools/detectors/kinetix.py:7

• PEP 8 recommends two blank lines between top-level definitions. Add a blank line between the imports and kinetix_factory() to keep module formatting consistent with the rest of the codebase.

from ophyd_async.epics.adkinetix import KinetixDetector
from ophyd_async.epics.adcore import ADWriterFactory

def kinetix_factory(num: int, path_provider, name: str):
    """Factory function to create a KinetixDetector with HDF writer."""

• Files reviewed: 18/19 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical lint and test-collection failures, along with unresolved API compatibility regressions, block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (10)

Previously missed (3) — in code that hasn't changed since the last review.

src/hextools/photon_delivery_system/dclm.py:149

• The public plan's positional API changed from change_energy(dclm, energy) to change_energy(energy, dclm=...) without a compatibility layer. Existing callers using the previous signature now pass a DCLM object as energy and fail during numeric validation; retain a compatible entry point or deliberately migrate all supported consumers.
  src/hextools/photon_delivery_system/slits.py:172
• For an invalid two-element value such as (10, 10), this condition calls len() on each float and raises TypeError instead of reaching the explicit ValueError below. Check that each element is a nested tuple before taking its length so malformed set values receive the documented validation error.
  tests/photon_delivery_system/test_slits.py:77
• The second slit test constructs the device with the same obsolete (prefix, number) call. It should use the new full prefix as well; otherwise this test uses different PVs from the startup device and can pass without validating the intended configuration.

src/hextools/motors.py:234

• Because these adjacent string literals are joined without whitespace, the resulting exception says matchingone of its names. That makes invalid objective input diagnostics misleading; include a trailing space after matching.

                f"Invalid objective value: {value}. "
                "Must be a CameraObjective or a string matching" \
                "one of its names."

src/hextools/motors.py:176

• This new class name contains underscores and lowercase words, which violates the repository's Ruff N801 CapWords class naming rule selected in pyproject.toml:140-156; the repository-wide pre-commit check will reject it. Rename it to a CapWords form (and update its startup/tests) or add an explicit, justified lint exception.

class FOV_2_4_mm_Camera(StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]):

src/hextools/photon_delivery_system/init.py:4

• The module split drops BeamMode and FilterSetting from the package-level API even though both were importable from hextools.photon_delivery_system before this refactor. Existing callers using those imports now fail; re-export them here or provide an explicit compatibility path.

from .dclm import DCLM, change_energy
from .filters import Filter, FilterPosition, load_filters

src/hextools/profiles/collection.py:30

• The newly added imports duplicate bpp and leave bp, bps, bpp, SuspendFloor, and show_docs unused because the corresponding scan/suspender/baseline code below is commented out. Ruff is run over the whole repository in CI, so this profile now fails with unused/redefinition errors; remove the unused imports or restore the code that consumes them.

from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp
from bluesky.suspenders import SuspendFloor
from hextools.utils import show_docs

src/hextools/profiles/collection.py:136

• These lines remove kinetix2, kinetix3, and kinetix4 from the collection profile; those detector objects were initialized before this change, so their globals and acquisition capability disappear at startup. This is an operational regression unrelated to splitting the PDS module unless the detector removal is intentional and coordinated.

    # kinetix2 = kinetix_factory(2, path_provider, name="kinetix2")
    # kinetix3 = kinetix_factory(3, path_provider, name="kinetix3")
    # kinetix4 = kinetix_factory(4, path_provider, name="kinetix4")

src/hextools/profiles/collection.py:170

• Commenting out fs_window removes the collection profile's configured fluorescence camera, so the new change_energy(..., fs_camera=...) auto-tuning path cannot be used from normal startup. Coordinate this operational change or keep the device wiring if the camera is expected to remain available.

    # fs_window = VimbaDetector(
    #     "XF:27IDA-BI{FS:1-Cam:1}",
    #     ADWriterFactory.hdf(path_provider),
    #     name="fs_window",
    #     plugins={"stats1": fs_window_stats},

tests/photon_delivery_system/test_slits.py:12

• Slits now takes an already-expanded PV prefix and a string name; it no longer accepts a numeric slit number. This call passes 1 as the name and creates XF:TEST:I}Mtr rather than the {Slt:1-Ax: PVs used by startup, so the fixture does not exercise the configured device. Use the expanded test prefix and a string name.

        device = Slits("XF:TEST:", 1)

• Files reviewed: 18/19 changed files
• Comments generated: 5
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Critical and moderate findings remain unresolved, including detector initialization, tomography cleanup, API compatibility, and test/lint failures.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (22)

src/hextools/detectors/germ.py:133

• The new public signal is misspelled as trigger_recieved, while the corresponding signal elsewhere in the same detector API is trigger_received. This exposes an inconsistent attribute name and makes callers likely to miss the signal; correct the spelling.

    channel: A[SignalRW[int], PvSuffix("Channel")]

src/hextools/detectors/phantom.py:533

• Completion now depends solely on selected_cine.cine_content_saved. The existing Phantom acquisition path and tests only advance download_count (tests/detectors/test_phantom.py:263-274) and never set this save signal, so once the counter stops this loop raises a timeout instead of completing. Preserve target-count completion or update the IOC/test contract and reset/observe the save state consistently.

                async for saved in observe_value(
                    selected_cine.cine_content_saved, done_timeout=DEFAULT_TIMEOUT
                ):
                    if saved:
                        return

src/hextools/detectors/phantom.py:500

• This new availability check runs before every Phantom download, but the existing PhantomAcquireLogic tests and mock callbacks do not initialize total_frame_count; the mock therefore reports no available frames and start_acquiring raises before issuing the download. Update all mock setups/full-stack callbacks to provide the new readback, or guard the check when that readback is unavailable.

        if total_download_frames > available_frames:
            raise RuntimeError(
                f"Requested {total_download_frames} frames to download, but only {available_frames} are available!"
            )

src/hextools/detectors/phantom.py:490

• The newly added comment misspells received; correct it so the acquisition logic documentation is clear.

        # If we recieved the trigger, we know at this point how many frames we'll have access to,

src/hextools/detectors/phantom.py:546

• This newly added print emits diagnostic output from the detector's timeout-recovery path during normal RunEngine operation. Use the project's logging mechanism or remove the debug output so detector plans do not write uncontrolled stdout.

                print(
                    f"Last value: {last_download_count}, Current value: {current}, Saved: {saved}"

src/hextools/motors.py:178

• Renaming DoubleObjCamera to FOV_2_4_mm_Camera removes an import still used by tests/test_motors.py:7 and its fixtures/tests, so test collection raises ImportError. Keep a compatibility alias or update all consumers in the same change.

class FOV_2_4_mm_Camera(
    StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]
):

src/hextools/motors.py:237

• The two adjacent string literals concatenate without a space, so invalid-objective errors say matchingone of its names, which is unclear to users.

            raise ValueError(
                f"Invalid objective value: {value}. "
                "Must be a CameraObjective or a string matching"
                "one of its names."

src/hextools/photon_delivery_system/init.py:6

• The package split drops BeamMode and FilterSetting from the package-level API. They were top-level names in the deleted photon_delivery_system.py, so existing from hextools.photon_delivery_system import ... callers now fail even though the classes still exist in submodules. Re-export them to preserve the package API.

from .dclm import DCLM, change_energy
from .filters import Filter, FilterPosition, load_filters
from .shutter import Shutter
from .slits import Slits

src/hextools/photon_delivery_system/slits.py:32

• The constructor now takes a complete PV prefix, but both new slit tests still call Slits("XF:TEST:", 1). That binds 1 to name and produces motor PVs such as XF:TEST:I}Mtr instead of the intended XF:TEST:{Slt:1-Ax:I}Mtr, so the tests do not exercise the configured device.

    def __init__(self, prefix: str, name: str = ""):

src/hextools/profiles/collection.py:25

• Path is a newly added unused import, so the all-files Ruff check will fail with F401. Remove it unless a real path annotation/use is added.

from pathlib import Path

src/hextools/profiles/collection.py:38

• The newly added bp, bps, bpp, SuspendFloor, and show_docs imports are not referenced by executable code; their only apparent uses are commented-out snippets. Ruff F401 is enabled and CI lints all files, so remove these dead imports or restore the code that uses them.

from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp
from bluesky.suspenders import SuspendFloor
from hextools.utils import show_docs

src/hextools/tomography/flyscans.py:161

• Cleanup also closes the photon shutter regardless of use_shutter, so disabling shutter handling still actuates hardware during finalization. Make this cleanup conditional on the same flag.

        yield from ensure_shutter_closed(photon_shutter, allow_actuation=True)

src/hextools/tomography/flyscans.py:12

• get_obj_from_ipython_ns is imported but never used; the plan resolves devices through ensure_available. Ruff F401 is enabled and this added import will fail the all-files lint job.

from hextools.utils import ensure_available, get_obj_from_ipython_ns

src/hextools/tomography/radiography.py:109

• These lookups run even when use_shutter=False, so the documented no-shutter mode still raises ValueError outside an IPython namespace. Resolve the optional shutters only inside the use_shutter branch, as their later use is already guarded.
  src/hextools/tomography/radiography.py:158
• The finalizer never executes this plan: calling a generator function without yield from returns a generator object, and _cleanup itself has no yield, so the photon shutter is not closed (and finalize_wrapper may receive None). Yield from the helper here.
  src/hextools/tomography/radiography.py:52
• nslsii.detectors and get_obj_from_ipython_ns are unused in this module; only AreaDetector, DetectorTrigger, TriggerInfo, the shutter helpers, and ensure_available are referenced. These added imports trigger Ruff F401 in the repository's all-files lint job.
  src/hextools/tomography/radiography.py:62
• This signature no longer accepts the front_end_shutter/photon_shutter positional arguments or the frames_per_burst, num_bursts, and wait_between_bursts keywords still used by tests/tomography/test_take_radiograph.py. Once the import is fixed, that test will fail with TypeError; update the test to the new API or preserve a compatibility path.
  src/hextools/utils.py:281
• indent is documented as an indentation level but is ignored, so every call renders at column zero. Pass an initial prefix derived from the requested indentation.

    _make_tree_body(x, device)

src/hextools/utils.py:7

• OrderedDict is imported but never used. Ruff F401 is enabled for this repository and runs on all files in CI, so this new import blocks the lint gate.

from typing import Any, OrderedDict, TypeVar

src/hextools/utils.py:193

• The new public verbose parameter controls whether connection failure reasons are printed, but it is missing from the function's Parameters section. Document its default and effect so callers can use the new behavior intentionally.

def auto_init_devices(timeout: float = 1.0, verbose: bool = False) -> DeviceProcessor:
    """Create a DeviceProcessor that connects devices, printing status for each.

    Parameters
    ----------
    timeout : float
        The timeout in seconds for each device connection attempt.

    Returns
    -------
    DeviceProcessor
        A DeviceProcessor that connects devices and prints their connection status.
    """

tests/detectors/test_germ.py:14

• GeRMDetector is imported in this new test module but never referenced; Ruff F401 will fail the all-files lint job. Remove the unused symbol from the import list.

    GeRMDetector,

tests/photon_delivery_system/test_slits.py:12

• The new Slits constructor takes a complete PV prefix and an optional string name; passing 1 as the second positional argument uses it as the device name and leaves the motor PVs as XF:TEST:I}Mtr, so this fixture no longer tests slit number 1. Pass the complete ...{Slt:1-Ax: prefix instead.

        device = Slits("XF:TEST:", 1)

• Files reviewed: 26/27 changed files
• Comments generated: 9
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved runtime compatibility, functional, and lint/test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (22)

Previously missed (1) — in code that hasn't changed since the last review.

src/hextools/tomography/radiography.py:110

• These lookups run even when use_shutter is left at its documented default of False, so a shutterless radiograph with the optional arguments omitted raises before the plan starts. Resolve the optional shutters only inside the use_shutter branch.

src/hextools/detectors/germ.py:5

• The new tkinter.font import is unused and tkinter is not a declared runtime dependency. Importing hextools.detectors.germ can therefore make detector/profile startup depend on an optional GUI module for no reason; remove the accidental import.

from collections.abc import Mapping, Sequence
from tkinter.font import names
from typing import Annotated as A

src/hextools/detectors/germ.py:310

• This unconditional print emits the detector livetime every time default trigger information is requested, polluting profile/RunEngine output. Remove the debug print and leave the value available through the returned TriggerInfo.

        print(livetime)

src/hextools/detectors/germ.py:370

• StandardDetector is initialized with only name, unlike the working PhantomDetector construction, which passes its driver, prefix, acquire logic, trigger logic, and writers to super().__init__. This will not construct a usable GeRM detector (and likely raises for the missing required driver); wire these components through the base constructor instead of calling add_detector_logics before base initialization.

        super().__init__(name=name)

src/hextools/detectors/phantom.py:500

• This new guard treats the mock/default total_frame_count of zero as authoritative, so the existing Phantom acquire tests—which set the requested download range but do not seed total_frame_count—now raise before starting the download. Update the test fixtures to provide the available frame count, or define how an unavailable readback should be handled instead of treating it as zero.

        available_frames, total_download_frames = await asyncio.gather(
            self.driver.total_frame_count.get_value(),
            self.driver.total_download_frames.get_value(),
        )
        if total_download_frames > available_frames:
            raise RuntimeError(
                f"Requested {total_download_frames} frames to download, but only {available_frames} are available!"
            )

src/hextools/detectors/phantom.py:521

• cines is created with keys 1..num_cines, but the existing mock setup leaves selected_cine at its default 0; this indexing therefore raises KeyError before wait_for_idle can observe completion. Normalize the IOC's cine numbering or handle an invalid/unset selection explicitly, and update the mock setup accordingly.

        selected_cine_num, last_download_count = await asyncio.gather(
            self.driver.selected_cine.get_value(),
            self.driver.download_count.get_value(),
        )
        selected_cine = self.driver.cines[selected_cine_num]

src/hextools/detectors/phantom.py:490

• Correct the spelling in this newly added comment.

        # If we recieved the trigger, we know at this point how many frames we'll have access to,

src/hextools/detectors/phantom.py:133

• This newly added signal name is misspelled as trigger_recieved, while the corresponding PhantomIO signal is trigger_received; the typo becomes part of the public PhantomCineIO API and is easy for callers to miss.

    trigger_recieved: A[SignalR[bool], PvSuffix("State_RBV.B3")]

src/hextools/detectors/phantom.py:533

• wait_for_idle now returns only when the selected cine's cine_content_saved becomes true, but the existing Phantom tests and full-stack mock callback still simulate completion solely by incrementing download_count (tests/detectors/test_phantom.py:263-274, 355-360). Those tests will time out even after all requested frames are downloaded; either retain a counter-based completion path or update the mocks and assertions to set the cine-saved signal.

                async for saved in observe_value(
                    selected_cine.cine_content_saved, done_timeout=DEFAULT_TIMEOUT
                ):
                    if saved:
                        return

src/hextools/motors.py:178

• Renaming DoubleObjCamera removes the symbol that the unchanged tests/test_motors.py still imports and annotates, causing test collection to fail with ImportError. Keep a backwards-compatible alias or update that test and other callers as part of this rename.

class FOV_2_4_mm_Camera(
    StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]
):

src/hextools/motors.py:231

• The documented string form does not work: "Right Objective".upper() becomes "RIGHT OBJECTIVE", which is compared to the enum name RIGHT_2MM, so the loop never converts the value and the following check raises. Normalize and compare both enum names and their wire values.

        if isinstance(value, str):
            for possible_value in CameraObjective:
                if value.upper() in possible_value.name:
                    value = possible_value
                    break

src/hextools/photon_delivery_system/init.py:4

• The old module exposed BeamMode and FilterSetting at hextools.photon_delivery_system, and the deleted test imported both from that root. Omitting them from the package exports breaks existing consumers during the module-to-package split; re-export the previously public types or explicitly provide a compatibility layer.

from .dclm import DCLM, change_energy
from .filters import Filter, FilterPosition, load_filters

src/hextools/photon_delivery_system/slits.py:36

• This constructor no longer accepts the old (prefix, num, name) form or inserts the {Slt:<num>-Ax: segment, but the new tests/photon_delivery_system/test_slits.py still calls Slits("XF:TEST:", 1). That passes 1 as the name and produces the wrong motor PVs (and may violate the device name type). Preserve the old signature or update all callers to pass the complete prefix.

    def __init__(self, prefix: str, name: str = ""):
        with self.add_children_as_readables(Format.CHILD):
            self.inboard = AsyncEpicsMotor(prefix + "I}Mtr")
            self.outboard = AsyncEpicsMotor(prefix + "O}Mtr")
            self.bottom = AsyncEpicsMotor(prefix + "B}Mtr")

src/hextools/profiles/collection.py:38

• This adds a second bpp import while also introducing bp/bps that are only referenced in comments; the profile now contains multiple unused/redefined imports. Ruff F401/F811 checks run on all Python files in CI, so the profile will fail linting before it can be used.

from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp
from bluesky.suspenders import SuspendFloor
from hextools.utils import show_docs

src/hextools/tomography/flyscans.py:67

• These lookups are unconditional even though fe_shutter and photon_shutter are optional and use_shutter can be False; consequently the documented no-shutter mode raises before resolving the actual scan devices. Guard these lookups with use_shutter.

    fe_shutter = ensure_available(Shutter, fe_shutter=fe_shutter)
    photon_shutter = ensure_available(Shutter, photon_shutter=photon_shutter)

src/hextools/tomography/flyscans.py:135

• This shutter actuation is unconditional, so use_shutter=False still dereferences and opens photon_shutter (which is allowed to be None). Put this operation behind the same use_shutter guard as the front-end check.

        yield from ensure_shutter_open(
            photon_shutter, allow_actuation=True, group="prepare", wait=False
        )

src/hextools/tomography/flyscans.py:161

• The finalizer also closes the photon shutter regardless of use_shutter, so disabling shutter handling still requires a shutter and can fail during cleanup. Guard the cleanup operation with use_shutter.

    def _cleanup():
        yield from ensure_shutter_closed(photon_shutter, allow_actuation=True)

src/hextools/tomography/flyscans.py:76

• Unlike take_radiograph, this path does not reject an acquire_period at or below exposure_time; it then constructs TriggerInfo(deadtime=acquire_period - exposure_time) with a zero or negative deadtime. Validate the period before preparing any devices.

    if acquire_period is None:
        acquire_period = exposure_time + FRAME_PERIOD_MARGIN

src/hextools/tomography/flyscans.py:163

• Passing _body and _cleanup here passes generator functions to finalize_wrapper, unlike radiography.py, which passes _body() and _cleanup(). The wrapper expects plan iterators, so this flyscan will fail when the RunEngine tries to execute the plan; call both functions before wrapping.

    yield from bpp.finalize_wrapper(_body, _cleanup)

src/hextools/utils.py:281

• The public indent argument is ignored: _make_tree_body is always called with an empty prefix, so print_device_tree(device, indent=2) produces the same output as indent=0 despite the docstring promising an indentation level.

    _make_tree_body(x, device)

tests/photon_delivery_system/test_slits.py:12

• Slits no longer accepts a numeric slit number: its second argument is now name, and the prefix must already include the {Slt:<n>-Ax: portion. Passing 1 here constructs the device with an integer name (and the wrong PVs), so this fixture cannot run against the new class.

        device = Slits("XF:TEST:", 1)

tests/photon_delivery_system/test_slits.py:77

• This second fixture repeats the removed (prefix, num) constructor form. The new Slits class expects a complete slit prefix and a string name, so this test also needs to use the new constructor.

        slits = Slits("XF:TEST:", 1)

• Files reviewed: 27/28 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved functional, compatibility, lint, and test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (23)

Previously missed (1) — in code that hasn't changed since the last review.

src/hextools/utils.py:7

• OrderedDict is newly imported but never used, so the repository's Ruff F401 check will fail on this file.

src/hextools/detectors/germ.py:370

• GeRMDetector registers its driver and detector logics before calling StandardDetector.__init__, then calls the base initializer without the driver, path provider, or data/writer configuration. The profile's GeRMDetector(...) construction will therefore fail or produce an uninitialized StandardDetector; initialize the base using the same contract as PhantomDetector.

        super().__init__(name=name)

src/hextools/detectors/germ.py:4

• This unused tkinter import adds an unrelated runtime dependency to the detector module and is rejected by the repository's Ruff F401 check. Remove it.

from tkinter.font import names

src/hextools/detectors/germ.py:310

• This unconditional debug print runs whenever the detector supplies default trigger information, polluting beamline console output during normal RunEngine operation. Remove it or route diagnostic information through the project's logging mechanism.

        print(livetime)

src/hextools/detectors/germ.py:133

• The new signal attribute is misspelled as trigger_recieved, while the corresponding driver signal is trigger_received. This creates an inconsistent public API and can make the cine trigger signal impossible to discover by its expected name.

    channel: A[SignalRW[int], PvSuffix("Channel")]

src/hextools/detectors/phantom.py:133

• The new public cine signal is misspelled trigger_recieved, while the corresponding driver signal is trigger_received; callers using the expected spelling cannot access this readback.

    trigger_recieved: A[SignalR[bool], PvSuffix("State_RBV.B3")]

src/hextools/detectors/phantom.py:500

• This new check makes existing Phantom acquisition tests fail before download because their mock setup does not set total_frame_count (the mock remains zero while frames are requested). Update the tests and acquisition fixtures to provide the available-frame readback, or ensure the check is only applied when that IOC readback is valid.

        available_frames, total_download_frames = await asyncio.gather(
            self.driver.total_frame_count.get_value(),
            self.driver.total_download_frames.get_value(),
        )
        if total_download_frames > available_frames:
            raise RuntimeError(
                f"Requested {total_download_frames} frames to download, but only {available_frames} are available!"
            )

src/hextools/detectors/phantom.py:532

• wait_for_idle now waits only for cines[selected_cine].cine_content_saved, but the unchanged Phantom tests simulate completion by incrementing download_count and never set this new signal. Those tests will time out; retain a valid download-counter completion path or update all fixtures to drive the cine-saved signal.

                async for saved in observe_value(
                    selected_cine.cine_content_saved, done_timeout=DEFAULT_TIMEOUT
                ):
                    if saved:

src/hextools/motors.py:178

• Renaming the public DoubleObjCamera class without an alias or updating all consumers leaves the unchanged tests/test_motors.py unable to import the class at collection time. Preserve a compatibility alias or include the required consumer/test migration in this PR.

class FOV_2_4_mm_Camera(
    StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]
):

src/hextools/motors.py:178

• These new class names use underscores and lowercase segments, which violates the repository's Ruff N801 CapWords rule and will fail the lint gate. Use CapWords class names (for example, FOV2_4mmCamera) and keep any screen-facing alias as a separate variable if needed.

class FOV_2_4_mm_Camera(
    StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]
):

src/hextools/photon_delivery_system/init.py:6

• The old hextools.photon_delivery_system module exposed BeamMode and FilterSetting, but the new package initializer omits both. Code using the existing top-level API will now fail with ImportError after this refactor; re-export these names and include them in __all__, or provide a compatibility path.

from .dclm import DCLM, change_energy
from .filters import Filter, FilterPosition, load_filters
from .shutter import Shutter
from .slits import Slits

src/hextools/photon_delivery_system/dclm.py:149

• This refactor changes the public positional call from the old change_energy(dclm, energy, ...) to change_energy(energy, dclm=...). Existing callers that pass the monochromator first will now treat a DCLM as the numeric energy and fail in np.isfinite; preserve the old positional order or add a compatibility wrapper while introducing the new keyword-based form.

def change_energy(
    energy: float,
    dclm: DCLM | None = None,
    fs_camera: AreaDetector | None = None,

src/hextools/profiles/collection.py:36

• This newly added grouped import rebinds bpp, which is already imported on line 13, and also introduces unused bp/bps names. Ruff's F811/F401 checks will reject the profile; keep one import and only the names used by executable code.

from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp

src/hextools/profiles/collection.py:219

• This direct assignment to the private _name attribute violates the repository's configured SLF001 lint rule and bypasses the public naming API. Use set_name, which the profile's device initialization code already uses.

perkin_elmer._name = "perkin-elmer"

src/hextools/tomography/flyscans.py:67

• These lookups are unconditional even though the API makes shutters optional and exposes use_shutter=False. A shutterless flyscan therefore fails before it can use the valid panda and motor arguments; resolve these devices only when shutter use is enabled.

    fe_shutter = ensure_available(Shutter, fe_shutter=fe_shutter)
    photon_shutter = ensure_available(Shutter, photon_shutter=photon_shutter)

src/hextools/tomography/flyscans.py:135

• The photon shutter is actuated unconditionally here, so use_shutter=False still dereferences and opens photon_shutter (which is allowed to be None). Guard this prepare-time operation with the same flag used for the front-end shutter.

        yield from ensure_shutter_open(
            photon_shutter, allow_actuation=True, group="prepare", wait=False
        )

src/hextools/tomography/flyscans.py:76

• An explicitly supplied acquire_period at or below exposure_time is not corrected or rejected, yet line 105 converts it into a negative TriggerInfo.deadtime. Apply the same validation/margin rule used by take_radiograph (or raise a clear error).

    if acquire_period is None:
        acquire_period = exposure_time + FRAME_PERIOD_MARGIN

src/hextools/tomography/flyscans.py:12

• get_obj_from_ipython_ns is newly imported but never used in this module, so Ruff F401 will reject the changed file.

from hextools.utils import ensure_available, get_obj_from_ipython_ns

src/hextools/tomography/radiography.py:110

• These lookups run even when use_shutter=False, so the documented shutter-disabled mode still raises if fe_shutter and photon_shutter are not supplied or present in IPython. Resolve the shutters only inside the use_shutter branch.

    fe_shutter = ensure_available(Shutter, fe_shutter=fe_shutter)
    photon_shutter = ensure_available(Shutter, photon_shutter=photon_shutter)

src/hextools/tomography/radiography.py:52

• The new nslsii.detectors and get_obj_from_ipython_ns imports are never referenced (the function uses its detectors argument and ensure_available instead). With Ruff F401 enabled, these imports will fail pre-commit and should be removed.

from nslsii import detectors
from ophyd_async.epics.adcore import AreaDetector
from ophyd_async.core import DetectorTrigger, TriggerInfo
from hextools.photon_delivery_system.shutter import ensure_shutter_closed, ensure_shutter_open
from hextools.utils import ensure_available, get_obj_from_ipython_ns

src/hextools/utils.py:281

• print_device_tree accepts an indent argument and documents it as the current indentation level, but this call discards the argument, so every requested indentation produces identical output.

    _make_tree_body(x, device)

tests/photon_delivery_system/test_slits.py:12

• The new Slits constructor takes the complete PV prefix, but this test still passes the old numeric num argument. It therefore supplies 1 as the device name and never exercises the intended {Slt:1-Ax: PV prefix.

        device = Slits("XF:TEST:", 1)

tests/photon_delivery_system/test_slits.py:77

• This second fixture still uses the removed (prefix, num) constructor, so the test does not construct the slit PVs through the new API.

        slits = Slits("XF:TEST:", 1)

• Files reviewed: 27/28 changed files
• Comments generated: 2
• Review effort level: Lite

[Anthony Sligar (sligara7)]

Work in progress. Will address CI checks failures and copilot comments in future PRs. All 4 main detectors are complete, mock tested and verified at HEX.

[Copilot]

🟡 Changes recommended

The flyscan finalizer and shutter handling contain runtime failures, with additional detector and compatibility issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (20)

src/hextools/tomography/flyscans.py:67

• The shutter arguments are optional when use_shutter=False, but these unconditional lookups still raise when neither shutter is supplied. Resolve the shutters only in the branch that uses them.

    fe_shutter = ensure_available(Shutter, fe_shutter=fe_shutter)
    photon_shutter = ensure_available(Shutter, photon_shutter=photon_shutter)

src/hextools/tomography/flyscans.py:135

• use_shutter=False is ignored here: the plan still opens the photon shutter. This defeats the flag and can actuate hardware for callers explicitly asking for a shutterless scan.

        yield from ensure_shutter_open(
            photon_shutter, allow_actuation=True, group="prepare", wait=False
        )

src/hextools/tomography/flyscans.py:161

• The finalizer also closes the photon shutter unconditionally, so disabling shutter use still changes shutter state on exit. Guard this cleanup with the same use_shutter condition as the open operation.

    def _cleanup():
        yield from ensure_shutter_closed(photon_shutter, allow_actuation=True)

src/hextools/tomography/flyscans.py:76

• A caller can pass an acquire_period shorter than exposure_time, producing a negative TriggerInfo.deadtime and invalid timing. Apply the same fallback/validation used by take_radiograph before constructing the trigger info.

    if acquire_period is None:
        acquire_period = exposure_time + FRAME_PERIOD_MARGIN

tests/photon_delivery_system/test_slits.py:77

• This second fixture repeats the obsolete (prefix, number) constructor call. Use the complete slit prefix and pass a string name so the test exercises the same PV layout as the profile.

        slits = Slits("XF:TEST:", 1)

src/hextools/motors.py:178

• Renaming this public class removes DoubleObjCamera, but tests/test_motors.py and existing callers still import and instantiate that name. The test suite will fail during collection with ImportError; retain a compatibility alias or update all consumers in this change.

class FOV_2_4_mm_Camera(
    StandardReadable, EpicsDevice, AsyncMovable[CameraObjective | str]
):

src/hextools/detectors/germ.py:370

• StandardDetector is initialized without the driver, acquire logic, trigger logic, or writer configuration. Unlike PhantomDetector, this leaves the detector base unconfigured (and may raise for missing required constructor arguments), so the profile's new germ device cannot be used. Initialize it through the StandardDetector constructor with self.driver, the logic objects, and the HDF writer/data logic.

        self.add_detector_logics(GeRMTriggerLogic(self.driver))
        self.add_detector_logics(GeRMAcquireLogic(self.driver))
        self.hdf = NDFileHDF5IO(prefix + "MCA1:HDF1:", name="hdf")
        self.add_detector_logics(
            ADHDFDataLogic(
                NDArrayDescription(
                    [self.driver.num_elements, self.driver.num_energy_bins],
                    self.driver.data_type,
                    self.driver.color_mode,
                ),
                path_provider,
                self.hdf,
            )
        )
        self.add_config_signals(*config_sigs)
        super().__init__(name=name)

src/hextools/utils.py:281

• The public indent parameter is documented as controlling the current indentation level, but it is ignored here. Calls such as print_device_tree(device, indent=2) produce the same output as indent=0. Pass an initial prefix based on the requested level.

    x = []
    _make_tree_body(x, device)

src/hextools/detectors/germ.py:4

• This newly added import is unused and also introduces a GUI-only dependency into the detector module. On headless installations without Tk, importing hextools.detectors.germ can fail before the detector is even constructed; remove it.

from tkinter.font import names

src/hextools/detectors/germ.py:310

• default_trigger_info() now prints the acquisition time on every call, which pollutes RunEngine/profile output and is not part of the detector API. Remove this debug print or replace it with appropriately gated logging.

    async def default_trigger_info(self) -> TriggerInfo:
        livetime = await self.driver.acquire_time.get_value()
        print(livetime)

src/hextools/tomography/flyscans.py:12

• get_obj_from_ipython_ns is imported but never used in this module. The repository's Ruff F401 check runs on all Python files, so this new import will fail CI.

from hextools.utils import ensure_available, get_obj_from_ipython_ns

src/hextools/tomography/radiography.py:48

• This new nslsii.detectors import is unused; the function parameter with the same name is used instead. Ruff F401 is enabled for all Python files, so remove this import to keep pre-commit passing.

from nslsii import detectors

src/hextools/tomography/radiography.py:52

• get_obj_from_ipython_ns is imported but never referenced in this module. Ruff F401 is enabled in the repository and will reject this new import during pre-commit.

from hextools.utils import ensure_available, get_obj_from_ipython_ns

src/hextools/profiles/collection.py:36

• This import block introduces bp, bps, and a duplicate bpp, but none is referenced by the active profile code. Ruff F401/F811 will fail pre-commit; remove the unused additions and the now-unused duplicate imports rather than leaving them in the startup module.

from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp

src/hextools/profiles/collection.py:29

• Path, NDStatsIO, and PluginSignalDataLogic are no longer referenced by the active profile after the related devices were commented out. These newly retained imports trigger Ruff F401 in the all-files pre-commit hook.

from pathlib import Path
from ophyd_async.epics.adcore import (
    ADWriterFactory,
    NDStatsIO,
    PluginSignalDataLogic,

src/hextools/profiles/collection.py:32

• The profile now constructs Kinetix devices through kinetix_factory, so this direct KinetixDetector import is unused and will fail Ruff F401 during pre-commit.

from ophyd_async.epics.adkinetix import KinetixDetector

src/hextools/profiles/collection.py:37

• SuspendFloor is imported but its only use is commented out below. Ruff F401 is enabled for the profile, so remove this now-unused import or restore an active suspender.

from bluesky.suspenders import SuspendFloor

src/hextools/profiles/collection.py:38

• show_docs is not referenced by the startup profile. This added unused import violates the repository's Ruff F401 check and prevents pre-commit from passing.

from hextools.utils import show_docs

src/hextools/detectors/germ.py:10

• DetectorDataLogic is no longer used after the old GeRM data-logic implementation was replaced. Ruff F401 will reject this unused import in CI.

    DetectorDataLogic,

src/hextools/photon_delivery_system/init.py:6

• The package no longer re-exports BeamMode or FilterSetting, although both were public names from the former hextools.photon_delivery_system module. Existing imports such as those in the removed monolithic-module tests will break after the module-to-package split; preserve these exports for compatibility.

from .dclm import DCLM, change_energy
from .filters import Filter, FilterPosition, load_filters
from .shutter import Shutter
from .slits import Slits

• Files reviewed: 27/28 changed files
• Comments generated: 5
• Review effort level: Lite