Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for box_size_um not included in robot_load_then_centre_plan. #763

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Jan 24, 2025

@rtuck99 rtuck99 marked this pull request as ready for review January 24, 2025 17:59
@rtuck99 rtuck99 requested a review from DominicOram January 24, 2025 17:59
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.00%. Comparing base (b663970) to head (b161fef).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
- Coverage   87.00%   87.00%   -0.01%     
==========================================
  Files         101      101              
  Lines        6966     6965       -1     
==========================================
- Hits         6061     6060       -1     
  Misses        905      905              
Components Coverage Δ
i24 SSX 72.84% <ø> (ø)
hyperion 96.53% <100.00%> (-0.01%) ⬇️
other 96.59% <100.00%> (+<0.01%) ⬆️

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. Just some minor nit comments. I made #768 to address them but you can just close that off if you like

Comment on lines +611 to +613
with patch(
"mx_bluesky.hyperion.experiment_plans.pin_centre_then_xray_centre_plan.detect_grid_and_do_gridscan"
) as mock_detect_grid:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I normally prefer using the patch decorator, unless there's a specific reason you can't (which I don't think there is in this case?) as it leads to less indentation and I feel like keeps the test tidier but I get that's probably personal preference

Comment on lines +304 to +305
"tests/test_data/parameter_json_files/good_test_grid_with_edge_detect_parameters"
".json",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Having the .json on a separate line made me think it was a new arg at first glance. I think it's more readable together, even if it is longer than the line length

@DominicOram DominicOram merged commit eed2654 into main Jan 27, 2025
22 checks passed
@DominicOram DominicOram deleted the 762_box_size_um_cannot_be_passed_from_gda branch January 27, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants