Vertical position derived signal for I23 Aithre goniometer #1164
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1164 +/- ##
=======================================
Coverage 97.97% 97.97%
=======================================
Files 199 199
Lines 7637 7649 +12
=======================================
+ Hits 7482 7494 +12
Misses 155 155 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 2a45ebc.
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Great, thanks. Some comments in code. Additionally, would you be able to make this into something generic and reuse it to do #1168 at the same time?
| async def test_vertical_position_set(goniometer: Goniometer): | ||
| await goniometer._set(value=5) | ||
| assert await goniometer.vertical_position.get_value() == 5 | ||
|
|
||
|
|
||
| async def test_vertical_position_get(goniometer: Goniometer): | ||
| assert goniometer._get(math.sqrt(3), 1, 30) == pytest.approx(2) |
There was a problem hiding this comment.
Should: I think rather than use the _get and _set (which are private) we should use the signals themselves. i.e. goniometer.vertical_position.set(...). I think we need a few more tests too:
- Test a getting and setting at few more angles/positions (
pytest.mark.parametrizemight help you here) - Test that setting the vertical actually changes the underlying signals as expected
| self.sampy = Motor(prefix + "SAMPY") | ||
| self.sampz = Motor(prefix + "SAMPZ") | ||
| self.omega = Motor(prefix + "OMEGA") | ||
| self.vertical_position = derived_signal_rw( |
There was a problem hiding this comment.
Should: I think some docstrings on what vertical_position means would be good.
Fixes DiamondLightSource/mx-bluesky#979
This functionality overlaps with #1215, but for reasons detailed in that PR, they are remaining separate at this time.
Instructions to reviewer on how to test:
dodal connect aithrestill connects the goniometerChecks for reviewer
dodal connect ${BEAMLINE}