Break up pds module into package, update slits device class, add slits to startup, update tests - #83
Open
Jakub Wlodek (jwlodek) wants to merge 5 commits into
Open
Break up pds module into package, update slits device class, add slits to startup, update tests#83Jakub Wlodek (jwlodek) wants to merge 5 commits into
Jakub Wlodek (jwlodek) wants to merge 5 commits into
Conversation
…s to startup, update tests
Contributor
There was a problem hiding this comment.
🟡 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_systeminto a package with separatedclm,filters,shutter, andslitsmodules (plus packagedfilters.yml). - Added a derived-signal-based
Slitsimplementation 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.
Comment on lines
+32
to
+37
| 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") | ||
| self.top = AsyncEpicsMotor(prefix + "T}Mtr") |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 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;Slitsnow expects(prefix, name=...). Update the instantiation to pass a stringnameand 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
Comment on lines
+77
to
+99
| def _get_description(self, in_pos: bool, motor_pos: float) -> str: | ||
| """Get the description of the current filter setting based on motor position. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| in_pos : int | ||
| Indicates whether the filter is in position. | ||
| motor_pos : float | ||
| Current position of the filter motor. | ||
|
|
||
| Returns | ||
| ------- | ||
| str | ||
| Description of the current filter setting. | ||
| """ | ||
| if not in_pos: | ||
| return "out of position" | ||
| closest = min( | ||
| self.positions.items(), key=lambda item: abs(item[1].position - motor_pos) | ||
| ) | ||
| return closest[1].description or closest[0].name.lower().replace("_", " ") | ||
|
|
||
| @AsyncStatus.wrap |
Comment on lines
+11
to
+12
| async with init_devices(mock=True): | ||
| device = Slits("XF:TEST:", 1) |
Contributor
There was a problem hiding this comment.
🟡 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
Slitsno longer accepts a numericnumargument (constructor isSlits(prefix: str, name: str = "")). This test still callsSlits("XF:TEST:", 1), which will raiseTypeErrorand 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 raiseTypeErrorwith the updatedSlitsAPI.
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_descriptionis currently defined at module scope, which prematurely ends theFilterclass block and leaves the subsequent indented@AsyncStatus.wrap/setmethod at an invalid indentation level. As-is, this file will fail to import with anIndentationError, andself._get_descriptionwon't exist onFilter.
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
Comment on lines
+3
to
+16
| from .dclm import DCLM, change_energy | ||
| from .filters import Filter, FilterPosition, load_filters | ||
| from .shutter import Shutter | ||
| from .slits import Slits | ||
|
|
||
| __all__ = [ | ||
| "Shutter", | ||
| "Filter", | ||
| "load_filters", | ||
| "FilterPosition", | ||
| "Slits", | ||
| "DCLM", | ||
| "change_energy", | ||
| ] |
Comment on lines
+173
to
+189
| coarse_angle_range : float | ||
| Half-width of the coarse pitch scan in degrees. | ||
| coarse_num_steps : int | ||
| Number of points in the coarse scan. | ||
| fine_angle_range : float | ||
| Half-width of the fine pitch scan in degrees. | ||
| fine_num_steps : int | ||
| Number of points in the fine scan. | ||
| photon_shutter : Shutter, optional | ||
| Shutter to close on exit. Falls back to the ``photon_shutter`` in the | ||
| IPython namespace when not provided. | ||
|
|
||
| Raises | ||
| ------ | ||
| RuntimeError | ||
| If the monochromator is not in monochromatic mode. | ||
| """ |
Contributor
There was a problem hiding this comment.
🟡 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
Slitswas updated to take just(prefix, name=...), but the test still constructs it with the old(prefix, num)signature, which will raiseTypeErrorand 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 newSlitsclass 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
Comment on lines
+3
to
+6
| from ophyd_async.epics.adkinetix import KinetixDetector | ||
| from ophyd_async.epics.adcore import ADWriterFactory | ||
|
|
||
| def kinetix_factory(num: int, path_provider, name: str): |
Comment on lines
+265
to
+268
| ps = PeakStats( | ||
| dclm.xtal2_pitch.name, | ||
| fs_camera.get_plugin_by_name(fs_stats_plugin_name, NDStatsIO).total.name, | ||
| ) |
Comment on lines
+169
to
+171
| value : tuple[tuple[float, float], tuple[float, float]] | ||
| ((horizontal_gap, horizontal_center), (vertical_gap, vertical_center)) | ||
| """ |
Comment on lines
+21
to
+29
| from pathlib import PureWindowsPath | ||
| from nslsii.ophyd_async.providers import NSLS2PathProvider | ||
| from ophyd_async.epics.adcore import ADWriterFactory, NDStatsIO, PluginSignalDataLogic | ||
| from ophyd_async.epics.adcore import ADWriterFactory, NDStatsIO, PluginSignalDataLogic, ContAcqDetector | ||
| from ophyd_async.epics.adkinetix import KinetixDetector | ||
| from ophyd_async.epics.advimba import VimbaDetector | ||
| from ophyd_async.fastcs.panda import HDFPanda | ||
| from tiled.client import from_uri, simple | ||
| from bluesky import plans as bp, plan_stubs as bps, preprocessors as bpp | ||
| from bluesky.suspenders import SuspendFloor |
Contributor
There was a problem hiding this comment.
🟡 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
Slitsconstructor now takes(prefix: str, name: str = ""), but the test passes an integer as the second argument (Slits("XF:TEST:", 1)), which will be treated asnameand 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 sobps.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
Comment on lines
203
to
206
| storage_ring.beam_current, | ||
| wb_slits, | ||
| pb_slits, | ||
| sample_tower, |
Comment on lines
+223
to
+229
| # Attempt to auto-deduce the CameraObjective from a string input. | ||
| if isinstance(value, str): | ||
| for possible_value in CameraObjective: | ||
| if value.upper() in possible_value.name: | ||
| value = possible_value | ||
| break | ||
|
|
Comment on lines
+231
to
+235
| raise ValueError( | ||
| f"Invalid objective value: {value}. " | ||
| "Must be a CameraObjective or a string matching" \ | ||
| "one of its names." | ||
| ) |
Comment on lines
+184
to
+194
| pe_path_provider = NSLS2PathProvider(RE.md, base_write_dir=PureWindowsPath("Z:\\proposals")) | ||
| perkin_elmer = ContAcqDetector( | ||
| "XF:27ID1-ES{PE-Det:1}", | ||
| ADWriterFactory.hdf(pe_path_provider), | ||
| name="perkin-elmer", | ||
| proc_suffix="Proc1:", | ||
| ) | ||
|
|
||
| # TODO: Figure out why the '-' character in the name is being | ||
| # replaced with '_' in the ctx manager | ||
| perkin_elmer._name = "perkin-elmer" |
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.
Uh oh!
There was an error while loading. Please reload this page.