526 do rotation scans#621
Conversation
Codecov Report
@@ Coverage Diff @@
## main DiamondLightSource/hyperion#621 +/- ##
==========================================
+ Coverage 89.75% 91.90% +2.14%
==========================================
Files 34 36 +2
Lines 1494 1630 +136
==========================================
+ Hits 1341 1498 +157
+ Misses 153 132 -21
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Review and comment on glaring things only, new issues created for smaller things |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Good start, some comments in code. I think it would have been a little cleaner to not have done any Nexus Callback stuff in this PR as we'll now end up with placeholder/half finished stuff on main but there's no point being a purist and removing it now. Given the rotation_scan_plan is a bit more scientist facing it would be good for it to have some docstrings. I think for the major stuff I've put it in new issues. Feel free to make other new issues for things (apart from the must comment).
| "env": { | ||
| "EPICS_CA_SERVER_PORT": "5066" | ||
| "EPICS_CA_SERVER_PORT": "5364" | ||
| }, |
There was a problem hiding this comment.
Should: This is specific to you, should remove it
There was a problem hiding this comment.
It's just the default for anyone
| from artemis.log import LOGGER | ||
|
|
||
|
|
||
| def arm_zebra(zebra: Zebra, timeout: float = 3): |
There was a problem hiding this comment.
I've added these to the growing list of nasty waits (https://github.com/DiamondLightSource/python-artemis/issues/426)
| """Set up the Zebra to collect a rotation dataset. Any plan using this is | ||
| responsible for setting the smargon velocity appropriately so that the desired | ||
| image width is achieved with the exposure time given here. | ||
|
|
||
| Parameters: | ||
| axis: I03 axes enum representing which axis to use for position | ||
| compare. Currently always omega. | ||
| start_angle: Position at which the scan should begin, in degrees. | ||
| scan_width: Total angle through which to collect, in degrees. | ||
| direction: 1 for positive direction or -1 for negative direction of | ||
| rotation. Other values cause a ValueError. Used for | ||
| adjusting the start angle based on shutter time. | ||
| shutter_time_and_velocity: tuple[float, float] representing the time it takes | ||
| (in seconds) for the shutter to open and the velocity of the | ||
| scan (in deg/s). Used to ajust the gate start so that |
There was a problem hiding this comment.
Nice commenting, thanks!
| # if shutter_time_and_velocity[0] != 0: | ||
| # shutter_time = shutter_time_and_velocity[0] | ||
| # velocity = shutter_time_and_velocity[1] | ||
| # start_angle += direction * (shutter_time * velocity) | ||
| # TODO FIX THIS HERE AND IN MAIN PLAN |
There was a problem hiding this comment.
Should: Move out into an issue and remove this comment (also probably shutter_time_and_velocity)
| def test_do_nothing(): | ||
| do_nothing() |
There was a problem hiding this comment.
Could: do_nothing is no longer used and was always a placeholder, we should remove it and this test
|
|
||
| # TODO planify these | ||
| eiger.set_detector_parameters(params.artemis_params.detector_params) | ||
| eiger.set_num_triggers_and_captures() |
There was a problem hiding this comment.
Should: Why are we calling this function here? It's done as part of stage
| "bluesky.preprocessors.__read_and_stash_a_motor", | ||
| __fake_read, | ||
| ): | ||
| # with patch.object(RE, "_read", MagicMock(return_value=0)): |
There was a problem hiding this comment.
Should: Remove commented out code
| ) | ||
| ) | ||
|
|
||
| assert mock_omega_sets.call_count == 4 |
There was a problem hiding this comment.
Should: This seems like an arbitrary assertion. Maybe better to assert the values are as expected
|
|
||
| @patch("artemis.experiment_plans.rotation_scan_plan.cleanup_plan") | ||
| @patch("bluesky.plan_stubs.wait") | ||
| def test_cleanup_happens( |
There was a problem hiding this comment.
Nit: I would split this into two tests, one with the failure and one without. Probably also add a third that confirms it cleans up when no error happens
| """Move an EpicsMotor 'axis' to angle 'start_angle', modified by an offset and | ||
| against the direction of rotation.""" | ||
| # can move to start as fast as possible | ||
| yield from bps.abs_set(axis.velocity, 120, wait=True) |
2 participants
Fixes #526
Implements a rotation_scan_plan and a few small support things necessary for it:
Requires dodal PR DiamondLightSource/dodal#49
To test: