Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #970 +/- ##
==========================================
+ Coverage 97.47% 97.48% +0.01%
==========================================
Files 147 148 +1
Lines 6208 6235 +27
==========================================
+ Hits 6051 6078 +27
Misses 157 157 ☔ View full report in Codecov by Sentry. |
23586f5 to
24da0be
Compare
olliesilvester
left a comment
There was a problem hiding this comment.
Looks good, more optional comments but feel free to ignore
src/dodal/devices/i03/beamstop.py
Outdated
| self.x = Motor(prefix + "X") | ||
| self.y = Motor(prefix + "Y") | ||
| self.z = Motor(prefix + "Z") |
There was a problem hiding this comment.
Maybe better to call these, eg, x_mm
There was a problem hiding this comment.
I originally modelled this after the I24 beamstop which has identically named PVs but maybe we should change them here and also look at whether we should update that one as well.
There was a problem hiding this comment.
Yeah, personally I always prefer to include the units in names, so in I24's too
There was a problem hiding this comment.
[When emitting bluesky documents] the units will come through as part of the descriptor document Configuration, straight from the EPICS motor record if they're configured there- is there a reason you want the units in the names here?
There was a problem hiding this comment.
It's just much easier to write plans when you can immediately see the units in code
There was a problem hiding this comment.
We've found quite a few cases where unit conversions have been the cause of a lot of confusion, areas of the code where we thought things were in one unit but actually in another. It's much easier to spot when a conversion up or down the unit scale is missing if signals and attributes are named clearly, and it's not a big overhead - if the unit changes, you have to change the code everywhere anyway so it's not a big deal.
It's a general preference across the team.
| robot load however all 3 resolution positions are the same. We also | ||
| do not use the robot load position in Hyperion. | ||
|
|
||
| Until we support moving the beamstop it is only necessary to check whether the |
There was a problem hiding this comment.
We could reference DiamondLightSource/mx-bluesky#484 in this comment too
a875604 to
e6ad77e
Compare
Part of fix for DiamondLightSource/mx-bluesky#698
This adds a beamstop device to the i03 beamline.
See also mx-bluesky PR 698
Instructions to reviewer on how to test:
dodal connect i03connects the beamstopChecks for reviewer
dodal connect ${BEAMLINE}