Add do fgs_plan #140

olliesilvester · 2024-08-05T13:50:31Z

Fixes #80 as part of https://github.com/orgs/DiamondLightSource/projects/6/views/1?pane=issue&itemId=73047222. This adds a slightly modified version of the do_fgs plan in Hyperion. Here, you can optionally device whether or not we need to do extra things - eg zocalo. This plan should be usable for VMXm, i03, i04, and hopefully other MX beamlines.

~~The idea of the level 1 folder is for it to be filled with plan stubs, i.e plans that cannot be ran on their own. I guess it could just be renamed to plan_stubs...~~ I renamed it plan_stubs

To test:

Check functionality is sensible, and compare to hyperion's do_fgs (https://github.com/DiamondLightSource/hyperion/blob/14421be796985f710129de1f105e2f1e65bf6a71/src/hyperion/experiment_plans/flyscan_xray_centre_plan.py#L329)
Confirm tests are sufficient

dperl-dls

I think we will probably want another level of categorisation than this, maybe put do_fgs into a gridscan module?

DominicOram

Thanks, I think we can maybe keep them even more similar

src/mx_bluesky/plan_stubs/do_fgs.py

DominicOram · 2024-08-12T19:39:39Z

src/mx_bluesky/plan_stubs/do_fgs.py

+    if post_plans:
+        yield from post_plans()


Should: Similar to above, I think we might be able to:

Wait for the zocalo stage in hyperion before calling do_fgs

Just always read the devices we have in both plans

So the wait for zocalo stage has the potential to take 2 seconds. I think maybe for the moment keep the post_plans bit and call it with a wait for stage on hyperion. I think it should be renamed to during_collection_plans though, as that's what it is

src/mx_bluesky/device_setup_plans/read_hardware_for_setup.py

… pre_plans

codecov · 2024-10-14T10:53:19Z

Codecov Report

Attention: Patch coverage is 98.78049% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.39%. Comparing base (afcde5b) to head (72677d5).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   78.28%   78.39%   +0.11%     
==========================================
  Files          93       96       +3     
  Lines        6815     6850      +35     
==========================================
+ Hits         5335     5370      +35     
  Misses       1480     1480

Components	Coverage Δ
i24 SSX	`57.08% <ø> (ø)`
hyperion	`96.65% <100.00%> (+0.17%)`	⬆️
other	`93.99% <98.64%> (-1.82%)`	⬇️

DominicOram

Sorry to put comments on this again. Is there a reason we didn't just put all of kickoff_and_complete_gridscan in common? I think it is common to all beamlines?

src/mx_bluesky/common/device_setup_plans/read_hardware_for_setup.py

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

src/mx_bluesky/common/plan_stubs/do_fgs.py

olliesilvester · 2024-10-22T15:23:28Z

Sorry to put comments on this again. Is there a reason we didn't just put all of kickoff_and_complete_gridscan in common? I think it is common to all beamlines?

No real reason, I was just trying to move bits incrementally, and wasn't sure about the Zocalo bits on other beamlines

DominicOram · 2024-10-22T15:35:52Z

Sorry to put comments on this again. Is there a reason we didn't just put all of kickoff_and_complete_gridscan in common? I think it is common to all beamlines?

No real reason, I was just trying to move bits incrementally, and wasn't sure about the Zocalo bits on other beamlines

I agree about the zocalo stuff not being needed everywhere but I think the other decorators are common, and would be needed for nexus/ispyb writing. If you want to do that in the next PR though that's fine

olliesilvester · 2024-10-22T16:01:42Z

OK, I'll put the whole function in common, minus some Zocalo bits

src/mx_bluesky/common/plan_stubs/do_fgs.py

olliesilvester · 2024-10-25T14:45:38Z

I started to reshuffle some of the hyperion tests in test_flyscan_xray_centre to the common test_do_fgs, but this quickly led into me having to move the whole parameter model + all callbacks into common too, so that is a job for a separate ticket

DominicOram

A couple of final questions around the tests but generally there.

tests/unit_tests/common/plan_stubs/test_do_fgs.py

DominicOram · 2024-10-29T11:40:10Z

tests/unit_tests/common/plan_stubs/test_do_fgs.py

+            "mx_bluesky.common.plans.do_fgs.check_topup_and_wait_if_necessary"
+        ) as mock_check_topup,
+        patch(
+            "mx_bluesky.common.plans.do_fgs.read_hardware_for_zocalo"
+        ) as mock_read_hardware,


Nit: Why not have these as a patch decorator? Seems slightly cleaner?

DominicOram · 2024-10-29T11:44:52Z

tests/unit_tests/common/plan_stubs/test_do_fgs.py

+        assert len(null_messages) == 0
+
+
+def test_kickoff_and_complete_gridscan_with_run_engine_correct_documents(


Could: It's probably better to test this with the real zocalo callback. Otherwise we end up potentially going down the rabbithole of #479

Yep, agreed. I did start to do this, but then it lead to having to move the callbacks to common, which requires also moving the parameters + constants to common, so I reverted. Anyway, I'll add a comment linking to an issue, saying to change this test once the zocalo callbacks have been moved

tests/unit_tests/common/plan_stubs/test_do_fgs.py

tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py

DominicOram

Great, thank you!

olliesilvester requested review from rtuck99 and dperl-dls August 5, 2024 13:51

dperl-dls reviewed Aug 6, 2024

View reviewed changes

DominicOram requested changes Aug 12, 2024

View reviewed changes

olliesilvester commented Aug 20, 2024

View reviewed changes

src/mx_bluesky/device_setup_plans/read_hardware_for_setup.py Outdated Show resolved Hide resolved

olliesilvester force-pushed the 80_add_do_fgs_plan branch from 7a76b60 to adb89b3 Compare August 20, 2024 14:44

olliesilvester added 5 commits August 20, 2024 16:29

old commit

ce4ab7b

Create general do_fgs plan stub with basic tests

c8ef9a0

Correct comment

9b91030

Move plan into a plans stubs folder

6aa5dae

Move Hyperion constants and read_hardware_for_zocalo plan, and remove…

b439f20

… pre_plans

olliesilvester force-pushed the 80_add_do_fgs_plan branch from adb89b3 to b439f20 Compare August 20, 2024 15:32

olliesilvester mentioned this pull request Aug 5, 2024

Investigate how to pull small pieces out of Hyperion into mx_bluesky for VMXm #216

Closed

Merge remote-tracking branch 'origin/main' into 80_add_do_fgs_plan

161ebec

olliesilvester added 7 commits October 14, 2024 11:55

move read_hardware_for_zocalo outside of hyperion

431a5d8

Move some constants out of hyperion

4c6b31e

Adjust tests

812b737

make sure plan is called with run open

addaae8

Restructure location of things

361103c

improve comments and tests

54d3894

add read hardware for zocalo test

3215aca

olliesilvester requested review from DominicOram and dperl-dls October 15, 2024 15:04

Move common parameters to correct location

3680210

DominicOram requested changes Oct 22, 2024

View reviewed changes

src/mx_bluesky/common/device_setup_plans/read_hardware_for_setup.py Show resolved Hide resolved

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py Outdated Show resolved Hide resolved

src/mx_bluesky/common/plan_stubs/do_fgs.py Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into 80_add_do_fgs_plan

2f2fe98

olliesilvester added 4 commits October 23, 2024 16:06

Move out larger section of FGS plan

acbfb73

Improve test, move tracing to common

c828ef5

Wait for zocalo queue to be empty before zocalo is triggered

124bfb4

Improved comments

8f0596e

olliesilvester commented Oct 23, 2024

View reviewed changes

src/mx_bluesky/common/plan_stubs/do_fgs.py Outdated Show resolved Hide resolved

olliesilvester added 5 commits October 23, 2024 17:12

FGS plan configures zocalo device metadata

268a036

Remove plan name as a parameter

20662ff

zocalo env as parameter, plan_stub is now a regular plan

8fb7cc1

Move log to correct line

73f5987

Add a few more tests

9c3512c

olliesilvester requested a review from DominicOram October 25, 2024 14:49

fix typing check

bcc1398

DominicOram requested changes Oct 29, 2024

View reviewed changes

olliesilvester and others added 2 commits October 29, 2024 15:46

Merge branch 'main' into 80_add_do_fgs_plan

9de7297

Review response

72677d5

olliesilvester requested a review from DominicOram October 30, 2024 09:25

DominicOram approved these changes Oct 30, 2024

View reviewed changes

DominicOram merged commit 4b4fdba into main Oct 30, 2024
22 checks passed

DominicOram deleted the 80_add_do_fgs_plan branch October 30, 2024 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add do fgs_plan #140

Add do fgs_plan #140

olliesilvester commented Aug 5, 2024 •

edited

Loading

dperl-dls left a comment

DominicOram left a comment

DominicOram Aug 12, 2024

DominicOram Aug 13, 2024

codecov bot commented Oct 14, 2024 •

edited

Loading

DominicOram left a comment

olliesilvester commented Oct 22, 2024

DominicOram commented Oct 22, 2024 •

edited

Loading

olliesilvester commented Oct 22, 2024 •

edited

Loading

olliesilvester commented Oct 25, 2024

DominicOram left a comment

DominicOram Oct 29, 2024

DominicOram Oct 29, 2024

olliesilvester Oct 30, 2024 •

edited

Loading

DominicOram left a comment

		assert len(null_messages) == 0


		def test_kickoff_and_complete_gridscan_with_run_engine_correct_documents(

Add do fgs_plan #140

Add do fgs_plan #140

Conversation

olliesilvester commented Aug 5, 2024 • edited Loading

dperl-dls left a comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

DominicOram Aug 12, 2024

Choose a reason for hiding this comment

DominicOram Aug 13, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

DominicOram left a comment

Choose a reason for hiding this comment

olliesilvester commented Oct 22, 2024

DominicOram commented Oct 22, 2024 • edited Loading

olliesilvester commented Oct 22, 2024 • edited Loading

olliesilvester commented Oct 25, 2024

DominicOram left a comment

Choose a reason for hiding this comment

DominicOram Oct 29, 2024

Choose a reason for hiding this comment

DominicOram Oct 29, 2024

Choose a reason for hiding this comment

olliesilvester Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

olliesilvester commented Aug 5, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

DominicOram commented Oct 22, 2024 •

edited

Loading

olliesilvester commented Oct 22, 2024 •

edited

Loading

olliesilvester Oct 30, 2024 •

edited

Loading