-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
add i18 beamline definition #722
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #722 +/- ##
==========================================
+ Coverage 95.63% 95.70% +0.06%
==========================================
Files 128 133 +5
Lines 5273 5399 +126
==========================================
+ Hits 5043 5167 +124
- Misses 230 232 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Stan! Why are there so many skipped devices here?
@DominicOram the devices are skipped as we don't need all of them at the moment, as we roll out the devices following the plans we make, so it's gradual and agile |
The more agile way to do it would be don't put the devices in at all until you need them otherwise we now have code in that never runs and we don't know if it works. |
we need them for the plans in a different repo. also aren't devices skipped all the time depending if they are off or taken out of a beamline physically? |
lint error
FAILED tests/devices/i22/test_dcm.py::test_configuration - AssertionError: Expected dcm to produce @iain-hall I'll try tackling the second one |
I would not use the Ophyd (i.e. not ophyd-async) SynAxis |
the |
that is because in ophyd-async h5py was added only as a dev dependency |
@DiamondJoseph how do I ask for the ophyd-async to be installed with the [sim] dependencies? the error in question: |
I'll answer on sSlack to avoid everyone getting emails |
waiting until pydantic 2 |
d9dc74a
to
033aec4
Compare
addressed
class SimDetector: | ||
def __init__(self, name: str, motor: Motor, motor_field: str): | ||
self.name = name | ||
self.motor = motor | ||
self.motor_field = motor_field | ||
self.pattern_generator = PatternGenerator( | ||
saturation_exposure_time=0.1, detector_height=100, detector_width=100 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you're redefining a SimDetector here rather than using the one in ophyd-async that uses the PatternGenerator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motor field isn't being used at all- is the PatternGenrator meant to read the motor and change its output depending on the value of the motor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure of the details - what do you think @iain-hall ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with PatternGenerator - could be useful. Any simulated detectors would be generally be used for offline testing rather than on the beamline, and we have one already for the diode used in the undulator gap/lookup table generator scan. Maybe we should only have real beamline devices in beamlines/i18.py, and add simulated devices as needed for offline testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the motor fields could be used in a plan with dot notation
getting an error when upgrading to a higher ophyd-async version ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
blueapi 0.6.1a3.dev20+gb6a47ba5.d20241113 requires bluesky==1.13.0a3, but you have bluesky 1.13 which is incompatible.
blueapi 0.6.1a3.dev20+gb6a47ba5.d20241113 requires event-model==1.21, but you have event-model 1.22.1 which is incompatible.
Successfully installed bluesky-1.13 dls-dodal-1.34.2.dev37+gd454384f1.d20241113 event-model-1.22.1 ophyd-async-0.8.0a4``` |
closed by misclick |
Fixes #709
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}