Add Jungfrau rotation scan plan #1234
Conversation
…ce/mx-bluesky into 1200_add_basic_jf_plans
|
@olliesilvester will integrate @ndevenish's path provider for now with an aim to get this merged ASAP |
…urce/mx-bluesky into 1200_add_jf_rotation_plan
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1234 +/- ##
==========================================
+ Coverage 92.28% 92.50% +0.22%
==========================================
Files 143 149 +6
Lines 8126 8314 +188
==========================================
+ Hits 7499 7691 +192
+ Misses 627 623 -4
🚀 New features to boost your workflow:
|
|
|
||
| def __init__(self): | ||
| def __init__(self, jf_writer: JungfrauCommissioningWriter): | ||
| self.jf_writer = jf_writer # For path information |
There was a problem hiding this comment.
I don't really like the addition of JungfrauCommissioningWriter to the callback.
Although the callback is currently in-process, adding it does prevent the callback being run externally. It seems like the only reason we have it is in order to access the final_path - is there any reason why it could not have been exposed as a signal on the writer and passed through in the event document?
I think that's cleaner as it also means that the event captures the value and means this event can be processed asynchronously.
There was a problem hiding this comment.
Sounds like a good idea, thanks
| "scan_points": [params.scan_points], | ||
| "rotation_scan_params": params.model_dump_json(), | ||
| } | ||
| # Should check topup gate here, but not yet implemented, |
There was a problem hiding this comment.
Are we actually currently using the external callbacks for JF?
There was a problem hiding this comment.
Nope, internal only for JF right now
|
I think this last batch of changes could probably have done with being another PR instead of making an already large PR larger. |
|
Discussed with @rtuck99 . We will merge this after addressing #1234 (comment). Likely to be lots of issues since this is a big change which hasn't been tested on the beamline for a while. We want to get it merged as it keeps getting stale |
| assert_allclose(actual_output[key], expected_output[key]) | ||
|
|
||
|
|
||
| async def test_assertion_error_if_no_jf_path_found( |
There was a problem hiding this comment.
feels like these two could share more code
rtuck99
left a comment
rtuck99
left a comment
There was a problem hiding this comment.
Looks good now, only minor comment about those two test functions.
Approved
Part of #1200
Prerequisites for CI to pass: DiamondLightSource/dodal#1721
Instructions to reviewer on how to test:
Checks for reviewer