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

Investigate: GDA should call Hyperion once per sample #202

Closed
DominicOram opened this issue Aug 16, 2024 · 3 comments
Closed

Investigate: GDA should call Hyperion once per sample #202

DominicOram opened this issue Aug 16, 2024 · 3 comments
Assignees
Labels

Comments

@DominicOram
Copy link
Contributor

We believe we now have all the logic for UDC in Hyperion. This should mean the GDA just needs to call Hyperion once for a sample to do the whole collection. However, we should investigate if there is anything else GDA is doing that we have missed, particularly for cases like S+C or Chi changes.

Acceptance Criteria

  • There are detailed tickets on what more we need to add into Hyperion to get it to do the whole collection in one call
@DominicOram DominicOram transferred this issue from DiamondLightSource/hyperion Sep 2, 2024
@DominicOram DominicOram self-assigned this Sep 11, 2024
@DominicOram
Copy link
Contributor Author

DominicOram commented Sep 11, 2024

Looking in i03's HCR on master:

  • Collection comes in to the execute_request
  • On line 382 this calls hyperion's execute_sample_loading - we should aim to replace this with a call to hyperion that does the whole of the rest of the function
  • execute_datacollection_prepare is then called. This firstly does centring, which we already have in hyperion.
  • We then check the cryo is in - I think the robot does this anyway so it's unnecessary confirm with scientist
  • There is then a section for plate screen mode - I think this is out of scope for hyperion confirm with scientist
  • We then take snapshots, move the backlight out and the aperture in - hyperion does this already
  • There is then some code around spectrum_taker, which is for metalID collections - I believe outside the scope of hyperion currently confirm with scientist
  • Sample position is then put in the comment - hyperion already does this
  • Energy/wavelength is then changed - hyperion can do this but needs stress testing
  • Beam size is set - we do this already in hyperion
  • Aperture added to comment - done in hyperion
  • execute_request then checks if the current is below some minimum value and waits for it to be above that - we don't do this in hyperion, maybe we should confirm with scientist
  • Then opens detector shutter, we do this in hyperion
  • Then calls do_sample where we first disables ROI mode, we do this in hyperion
  • Then sets transmission to 1 and locks XBPM - hyperion already does this straight after XRC
  • Then some dose scaling - I don't think this is turned on for i03 but we will need it at some point confirm with scientist
  • Then changing transmission - hyperion already does this but should do it better see Use transmission_and_xbpm_feedback_for_collection_wrapper for rotation scans #205
  • Then sets some parameters on the detector - hyperion already does this
  • Then starts looping through the oscillations - I think this is always only one collection for UDC confirm with scientist
  • Then clears odin errors, hyperion already does this
  • Then triggers zocalo - hyperion already does this
  • Then restore_stub_offsets, which we are not using for now
  • Then in the finally turns xbpm feedback on again - Use transmission_and_xbpm_feedback_for_collection_wrapper for rotation scans #205 will fix this

@DominicOram
Copy link
Contributor Author

DominicOram commented Sep 11, 2024

Discussed with @neil-i03 with the following conclusions:

  1. Cryo in check is not required
  2. Plate screening is outside the scope of hyperion but we should check that we're not accidentally doing it Throw error if hyperion started in plate screening mode #467
  3. Metal ID can be ignored for now, we will come up with a proper solution to it in the future
  4. The ring current check is actually only checking if the current is >0 at the moment. This should be caught by xbpm feedback anyway so no need to add this
  5. There is some dose scaling on i03 currently but it's handled by Agamemnon, we do not need to worry about it.
  6. Multiple oscillations are only used for stepped transmission collections, these are an edge case we can handle separately, see Ensure hyperion behaves well with stepped transmission collections #471

In conclusion to do this we will need to do, in roughly this order:

@DominicOram
Copy link
Contributor Author

This is now being implemented so calling the issue donw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant