-
Notifications
You must be signed in to change notification settings - Fork 4
1318 optimise oav transmission new #1473
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
Conversation
… and the frac_of_max + add docstrings
DominicOram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes for myself. I will address these
| Args: | ||
| devices: These are the specific ophyd-devices used for the plan, the | ||
| defaults are always correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: Not sure I like having the args not matching the actual names of the args. Also, there are no defaults and group is not a device
| max_pixel: MaxPixel = inject("max_pixel"), | ||
| attenuator: BinaryFilterAttenuator = inject("attenuator"), | ||
| ): | ||
| yield from bps.mv(attenuator, 100) # 100 % transmission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must: Attenuator takes transmission as a fraction, e.g. 0-1
| f"Testing transmission {mid}, brightest pixel found {brightest_pixel}" | ||
| ) | ||
|
|
||
| if target_pixel_l - tolerance < brightest_pixel < target_pixel_l + tolerance: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: I think there is the (very slim) potential for this to get stuck if brightest_pixel=target_pixel_l - tolerance or brightest_pixel=target_pixel_l + tolerance.
|
An updated PR for the same changes is here - a lot of the comments here are still relevant though |
Fixes #1318
Instructions to reviewer on how to test:
Need to add tests.
Checks for reviewer