Skip to content
This repository was archived by the owner on Sep 2, 2024. It is now read-only.

Full grid scan sets transmission, transmission sent to ispbyb#753

Merged
DominicOram merged 10 commits into
mainfrom
716_full_grid_scan_sets_transmission_first
Jul 17, 2023
Merged

Full grid scan sets transmission, transmission sent to ispbyb#753
DominicOram merged 10 commits into
mainfrom
716_full_grid_scan_sets_transmission_first

Conversation

@olliesilvester

@olliesilvester olliesilvester commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

Fixes #716 and fixes #744

To test:

  1. Confirm both requirements are met
  2. Run tests

@olliesilvester

Copy link
Copy Markdown
Contributor Author

When this is merged, the GDA code currently handling this should be removed

@olliesilvester olliesilvester changed the title 716 full grid scan sets transmission first Full grid scan sets transmission Jun 22, 2023
@olliesilvester olliesilvester changed the title Full grid scan sets transmission Full grid scan sets transmission, transmission sent to ispbyb Jun 22, 2023
@olliesilvester
olliesilvester force-pushed the 716_full_grid_scan_sets_transmission_first branch from bd09fe8 to 15730bd Compare June 28, 2023 12:13
@olliesilvester
olliesilvester marked this pull request as ready for review June 28, 2023 12:19
@codecov

codecov Bot commented Jun 28, 2023

Copy link
Copy Markdown

Codecov Report

Merging #753 (329a9e0) into main (8bd7078) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 329a9e0 differs from pull request most recent head c31703c. Consider uploading reports for the commit c31703c to get more accurate results

@@            Coverage Diff             @@
##             main     #753      +/-   ##
==========================================
- Coverage   93.30%   93.28%   -0.02%     
==========================================
  Files          37       37              
  Lines        1866     1861       -5     
==========================================
- Hits         1741     1736       -5     
  Misses        125      125              
Impacted Files Coverage Δ
...rc/artemis/experiment_plans/fast_grid_scan_plan.py 93.50% <100.00%> (+0.17%) ⬆️
src/artemis/experiment_plans/full_grid_scan.py 100.00% <100.00%> (ø)
...ternal_interaction/callbacks/fgs/ispyb_callback.py 98.03% <100.00%> (+0.03%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, thanks. I think we need to change the API to make it clearer this is now the transmission we want to set, not the readback. I suggest adding transmission to the experiment params for external API instead of ispyb params (but keep the ones in ispyb params for internal use)

Comment on lines +81 to +85
yield from bps.abs_set(
attenuator, parameters.artemis_params.ispyb_params.transmission
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Must: We need to wait on this to be done in the gridscan. In fact it falls under the same sort of wait as arming, I think we rename arming (here and in the wait) to something like "ready_for_data_collection" then put the set_transmission in the same group

yield from read_hardware_for_ispyb(und, syn, slits, attn)
yield from bps.close_run()

RE(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should: We should add transmission to the test values and assertions here

@olliesilvester
olliesilvester force-pushed the 716_full_grid_scan_sets_transmission_first branch from 15b66bd to 48e15fa Compare July 3, 2023 10:10
@olliesilvester

olliesilvester commented Jul 3, 2023

Copy link
Copy Markdown
Contributor Author

Right now the tests are failing due to a new Pydantic version, the same as #758 (review)

@DominicOram

Copy link
Copy Markdown
Collaborator

Right now the tests are failing due to a new Pydantic version, the same as #758 (review)

Should now be fixed with main merged in. I will re-review.

@DominicOram
DominicOram self-requested a review July 5, 2023 16:16

@DominicOram DominicOram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great, thank you

@DominicOram
DominicOram merged commit 6c618d7 into main Jul 17, 2023
@DominicOram
DominicOram deleted the 716_full_grid_scan_sets_transmission_first branch July 17, 2023 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get transmission for ispyb from the PV Full grid scan should set transmission first

2 participants