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

Read hardware on every read of the aperture scatterguard #789

Merged
merged 12 commits into from
Sep 27, 2024

Conversation

DominicOram
Copy link
Contributor

@DominicOram DominicOram commented Sep 14, 2024

Fixes #782

Instructions to reviewer on how to test:

  1. Confirm tests now all pass
  2. Confirm docs make sense

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

Copy link

codecov bot commented Sep 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (936dfaa) to head (de7b423).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #789      +/-   ##
==========================================
+ Coverage   94.99%   95.01%   +0.01%     
==========================================
  Files         115      116       +1     
  Lines        4737     4753      +16     
==========================================
+ Hits         4500     4516      +16     
  Misses        237      237              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/dodal/common/signal_utils.py Outdated Show resolved Hide resolved
src/dodal/common/signal_utils.py Outdated Show resolved Hide resolved
src/dodal/devices/aperturescatterguard.py Outdated Show resolved Hide resolved
@coretl
Copy link
Collaborator

coretl commented Sep 18, 2024

This is better than what was there originally, but I'm concerned that you don't override subscribe so you have a Signal where get_value gives you an enum, but subscribe gives you a float.

I wonder if there is a case for supplying both a forward and inverse transform, that operate on values rather than Signals, and making a DerivedSignalBackend that applies the transforms on get, put and subscribe?

@DominicOram
Copy link
Contributor Author

This is better than what was there originally, but I'm concerned that you don't override subscribe so you have a Signal where get_value gives you an enum, but subscribe gives you a float.

Yh, I agree but this is going to be hard. I guess the correct solution is to subscribe to all the signals you derive from then recalculate the derivation and push an update when any of them change.

I wonder if there is a case for supplying both a forward and inverse transform, that operate on values rather than Signals, and making a DerivedSignalBackend that applies the transforms on get, put and subscribe?

Yep, I think we will need these in the generic case.

Can we push the full solution to bluesky/ophyd-async#525 and treat this as a first pass example of how you could do it but with it's limitations? I would be happy to help contribute to the full solution but given it's a reasonably large feature, with quite a few stakeholders it will take a while and I would rather not have this bug in hyperion in the mean time.

@coretl
Copy link
Collaborator

coretl commented Sep 19, 2024

Can we push the full solution to bluesky/ophyd-async#525 and treat this as a first pass example of how you could do it but with it's limitations? I would be happy to help contribute to the full solution but given it's a reasonably large feature, with quite a few stakeholders it will take a while and I would rather not have this bug in hyperion in the mean time.

Sounds good to me, could we have a comment to this effect in the code please?

Copy link
Collaborator

@noemifrisina noemifrisina left a comment

Choose a reason for hiding this comment

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

Looks like comments have been addressed.
Thanks, this is useful!

@DominicOram DominicOram merged commit 228d9dc into main Sep 27, 2024
18 checks passed
@DominicOram DominicOram deleted the 782_read_aperture_scatterguard_better branch September 27, 2024 16:12
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.

Read aperture/scatterguard hardware on read
4 participants