Fix for shutter renaming#1742
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1742 +/- ##
=======================================
Coverage 92.87% 92.87%
=======================================
Files 156 156
Lines 8563 8563
=======================================
Hits 7953 7953
Misses 610 610
🚀 New features to boost your workflow:
|
| from dodal.devices.xbpm_feedback import XBPMFeedback | ||
| from dodal.devices.zebra.zebra import Zebra | ||
| from dodal.devices.zebra.zebra_controlled_shutter import ZebraShutter | ||
| from dodal.devices.zebra.zebra_controlled_shutter import MXZebraShutter |
There was a problem hiding this comment.
I think MX should be Mx - but I guess here is not the place to argue that
There was a problem hiding this comment.
I would argue that MX is acronym, and as discussed in DiamondLightSource/dodal#2033 (comment), acronyms are all upper case in PEP8. I don't mind having the discussion but:
- It should be in a wider group
- If we change away from PEP8 we should then document the exception and fix it everywhere at once
| "current, expected", | ||
| [ | ||
| # exact multiples | ||
| (0, 0), |
There was a problem hiding this comment.
if the numerical type is float rather than pure int You should have some non-integer test examples
There was a problem hiding this comment.
As above, this was an accidental change, I will remove
| (-10, 0), | ||
| (-200, -360), | ||
| # halfway cases | ||
| (180, 0), |
There was a problem hiding this comment.
I would have made these test case close to an edge like 180.1 or 179.9
balancing on the exact knife edge is inviting the test to go flaky and really nobody would care which way exactly 180 went ( when rounded ) in practice
There was a problem hiding this comment.
As above, this was an accidental change, I will remove
|
|
||
| def find_nearest_omega_360(gonio: XYZOmegaStage): | ||
| current_omega = yield from bps.rd(gonio.omega.user_readback) | ||
| return round(current_omega / 360) * 360 |
There was a problem hiding this comment.
does it not make sense to put the maths into dodal ?
and test it there?
There was a problem hiding this comment.
Apologies, this was accidentally commited by me. Not sure why it made it made it here
| "ophyd-async >= 0.16.0", | ||
| "bluesky >= 1.14.6", | ||
| "dls-dodal @ git+https://github.com/DiamondLightSource/dodal.git@main", | ||
| "dls-dodal @ git+https://github.com/DiamondLightSource/dodal.git@cryst_bluesky_10_fast_shutter", |
There was a problem hiding this comment.
again we're pinning to a specific version of dodal - is that an intended side effect of this PR ?
CoePaul
left a comment
CoePaul
left a comment
There was a problem hiding this comment.
Just commented on some things I noticed
2 participants
Update to match DiamondLightSource/dodal#2053
Instructions to reviewer on how to test:
Checks for reviewer