Add ability to move scintillator in to beam for i04 #1583
Conversation
…to just In and Out.
srishtysajeev
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
+ Coverage 98.77% 98.80% +0.02%
==========================================
Files 246 250 +4
Lines 8845 9003 +158
==========================================
+ Hits 8737 8895 +158
Misses 108 108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/dodal/devices/scintillator.py
Outdated
| elif all( | ||
| isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) | ||
| for axis_pos, axis_in_beam, axis_tolerance in zip( | ||
| current_pos, | ||
| self._scintillator_in_yz_mm, | ||
| self._yz_tolerance_mm, | ||
| strict=False, | ||
| ) | ||
| ): |
There was a problem hiding this comment.
Should: Can you pull this out into a function? Something like:
def check_at_position(current_position: tuple[float, float], position_to_check: tuple[float, float]):
return all(
isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance)
for axis_pos, axis_in_beam, axis_tolerance in zip(
current_position,
position_to_check,
self._yz_tolerance_mm,
strict=False,
)
)Then use it here and above
There was a problem hiding this comment.
should the function be a 'hidden function' I mean with the '_' at the start?
There was a problem hiding this comment.
Discussed in person. Yes it should be private
There was a problem hiding this comment.
Done that now - made them all private
src/dodal/devices/scintillator.py
Outdated
| if ( | ||
| self._aperture_scatterguard().selected_aperture.get_value() | ||
| != ApertureValue.PARKED | ||
| ): | ||
| raise ValueError( | ||
| "Cannot move scintillator in if aperture/scatterguard is not parked" | ||
| ) |
There was a problem hiding this comment.
Could: Can you pull this into a function of something like check_aperture_parked?
| await scintillator.selected_pos.set(InOut.IN) | ||
|
|
||
| assert await scintillator.y_mm.user_setpoint.get_value() == 100.855 | ||
| assert await scintillator.z_mm.user_setpoint.get_value() == 101.5115 | ||
|
|
There was a problem hiding this comment.
Should: It's better to put this in it's own test, because then it's more obvious what exactly is failing. This test name also references it being set to out but here you're setting it to in
There was a problem hiding this comment.
We should also probably be using the assert_value testing function from ophyd_async e.g
from ophyd_async.testing import assert_value
...
await assert_value(scintillator.y_mm.user_setpoint, 100.855)
Fixes #1567
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}