Add detector ID to eiger #1786
Add detector ID to eiger #1786
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1786 +/- ##
=======================================
Coverage 99.12% 99.12%
=======================================
Files 282 282
Lines 10683 10687 +4
=======================================
+ Hits 10589 10593 +4
Misses 94 94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, couple of small comments, otherwise looks good
src/dodal/devices/eiger.py
Outdated
| arming_status.set_finished() | ||
|
|
||
| def __init__(self, beamline: str = "i03", *args, **kwargs): | ||
| def __init__(self, beamline: str = "i03", detector_id: int = 78, *args, **kwargs): |
There was a problem hiding this comment.
Nit: There's not really a reason that 78 should be the default, that's just what is on i03. I don't think there is a sensible default.
There was a problem hiding this comment.
Should I remove the beamline default too?
There was a problem hiding this comment.
Yes, I think so. I'm not even 100% sure where it is used but given we're about to move eiger anyway that's a rabbit hole that we probably don't want to go down
src/dodal/devices/eiger.py
Outdated
| self.beamline = beamline | ||
|
|
||
| self._detector_id = detector_id | ||
| self.detector_id = AttributeSignal( |
There was a problem hiding this comment.
Could: This is the id used in ispyb so should probably have ispyb in variable names so people know what it is.
jacob720
force-pushed
the
mx_bluesky_1472_add_detector_id_to_eiger
branch
from
09a2b5d to
abce9f1
Compare
|
Made some changes to the defaults and how detector ID is set because you cannot instantiate a device with parameters using |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Minor comment, take it or leave it
| super().__init__(*args, **kwargs) | ||
| self.beamline = beamline | ||
|
|
||
| self.detector_id = ispyb_detector_id |
There was a problem hiding this comment.
Nit: Would be good to call this ispyb_detector_id too
There was a problem hiding this comment.
The AttributeSignal is already called ispyb_detector_id so it would have to have a different name
Fixes DiamondLightSource/mx-bluesky#1472
Required by DiamondLightSource/mx-bluesky#1526
Adds detector ID signal to eiger.
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}