Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
- Coverage 92.85% 92.75% -0.11%
==========================================
Files 153 150 -3
Lines 8638 8377 -261
==========================================
- Hits 8021 7770 -251
+ Misses 617 607 -10
🚀 New features to boost your workflow:
|
rtuck99
added
dev experience
rtuck99
force-pushed
the
1532_remove_gdarunner
branch
from
1089993 to
943ae89
Compare
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Great, thank you! Couple of comments on how we could go further but this is a lot nicer now.
Could: Many of the plans that were in experiment_registry are now not used e.g. pin_tip_centre_then_xray_centre, rotation_scan, hyperion_grid_detect_then_xray_centre. Whilst they will be useful endpoints again at some point I think we should remove them now as they are distracting. In fact we had an issue recently where we were modifying the wrong one.
| """Main application entry point.""" | ||
| args = parse_cli_args() | ||
| initialise_globals(args) | ||
| hyperion_port = HyperionConstants.HYPERION_PORT |
There was a problem hiding this comment.
Nit: This would now make more sense being called WATCHDOG_PORT rather than HYPERION_PORT
There was a problem hiding this comment.
I think that would also be confusing - the watchdog port is different when running in blueapi mode - the callbacks talk to the supervisor not blueapi.
src/mx_bluesky/hyperion/runner.py
Outdated
There was a problem hiding this comment.
Could: Now that PlanRunner is the only implementation of this we can merge the two
| "hyperion_grid_detect_then_xray_centre": { | ||
| "setup": hyperion_grid_detect_then_xray_centre_plan.create_devices, | ||
| "param_type": GridScanWithEdgeDetect, | ||
| }, | ||
| "pin_tip_centre_then_xray_centre": { | ||
| "setup": pin_centre_then_xray_centre_plan.create_devices, | ||
| "param_type": PinTipCentreThenXrayCentre, | ||
| }, | ||
| "rotation_scan": { | ||
| "setup": rotation_scan_plan.create_devices, |
There was a problem hiding this comment.
Could: Only the load_centre_collect create_devices is now used. All the others can be removed. In fact there's a few that weren't even referenced before this
| raise PlanNotFoundError(f"Experiment plan '{plan_name}' not found in registry.") | ||
|
|
||
| experiment_internal_param_type = experiment_registry_entry.get("param_type") | ||
| plan = context.plan_functions.get(plan_name) |
There was a problem hiding this comment.
Could: We are now never using the plans loaded into the context, only the devices. To avoid potential confusion it would be good to remove the context.with_plan_module(hyperion_plans) in setup_context
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Great, thank you! Couple of comments on how we could go further but this is a lot nicer now.
Could: Many of the plans that were in experiment_registry are now not used e.g. pin_tip_centre_then_xray_centre, rotation_scan, hyperion_grid_detect_then_xray_centre. Whilst they will be useful endpoints again at some point I think we should remove them now as they are distracting. In fact we had an issue recently where we were modifying the wrong one.
rtuck99
force-pushed
the
1532_remove_gdarunner
branch
from
f5c1a2f to
9f0b839
Compare
rtuck99
force-pushed
the
1532_remove_gdarunner
branch
from
9f0b839 to
7b74a5f
Compare
2 participants
Fixes
Link to dodal PR (if required): #N/A
(remember to update
pyproject.tomlwith the dodal commit tag if you need it for tests to pass!)Instructions to reviewer on how to test:
Checks for reviewer