Beam attenuation#35
Conversation
…ng tests; ignored eiger_async tests for now so that running tests is somewhat useful
Jakub Wlodek (jwlodek)
left a comment
Jakub Wlodek (jwlodek)
left a comment
There was a problem hiding this comment.
Marked some suggestions. Mainly I'd recommend making both device classes Movables and re-work the current methods for changing states into a set method override.
…syncStatus, upgraded version of ophyd-async because of test methods used, and to match what is in cdi-profile-collection
…d-async is incompatible with it
…ot have to be zero-indexed
…ed xrayutilities to calculate the transmission
… bank is the transmission
| fetch-depth: 0 | ||
| persist-credentials: false | ||
|
|
||
| - uses: hynek/build-and-inspect-python-package@c52c3a4710070b50470d903818a7b25115dcd076 # v2.13.0 |
There was a problem hiding this comment.
Please put the SHA pinning back.
| return 1 - self.transmission | ||
|
|
||
|
|
||
| THICKNESSES = (16, 24, 66, 124) # microns |
There was a problem hiding this comment.
I would make each of these a combination of (material, thickness), the generalization is pretty easy to do up front.
| class Attenuator(EpicsDevice, AsyncMovable[AttenuatorStatusEnum]): | ||
| filter_material = xu.materials.Al | ||
|
|
||
| def __init__(self, prefix: str, num: int, thickness: int): |
There was a problem hiding this comment.
I would type this as float.
|
|
||
| def __init__(self, prefix: str, num: int, thickness: int): | ||
| """ | ||
| prefix - the common prefix for the attenuator bank |
There was a problem hiding this comment.
Please use numpydoc conventions
| i: Attenuator(self.prefix, i, self.thicknesses[i - 1]) | ||
| for i in range(1, len(self.thicknesses) + 1) |
There was a problem hiding this comment.
| i: Attenuator(self.prefix, i, self.thicknesses[i - 1]) | |
| for i in range(1, len(self.thicknesses) + 1) | |
| i: Attenuator(self.prefix, i, thickness) | |
| for i, thickness in enumerate(self.thicknesses, start=1) |
Slightly more idiomatic Python
|
|
||
| @property | ||
| def photon_energy(self): | ||
| return self.energy.energy.readback.get() |
There was a problem hiding this comment.
after a lot of back-and-forth we have avoided wrapping epics I/O in properties. The main arguments against are that it violates the expectation that properties are attributes which are fast (not arbitrarily long) and is not uniform across other devices.
| def egu(self): | ||
| return self.energy.egu | ||
|
|
||
| async def get_status(self): # type: ignore[reportUnknownParameterType] |
There was a problem hiding this comment.
should this be async def read ? That way we can toss this in a baseline list and it will "just work".
| """ | ||
| status = {} | ||
| active_attens = [] | ||
| en = self.photon_energy |
There was a problem hiding this comment.
we are already in an async function here so
| en = self.photon_energy | |
| en = await self.energy.readback.value() |
I may be wrong about the exact name, ophyd-async has a verb for "get me the one true number for this or explode".
|
|
||
| def _calculate_total_transmission(self, *attenuators: Attenuator) -> float: | ||
| transmissions = [ | ||
| a.transmission(self.photon_energy, self.egu) for a in attenuators |
There was a problem hiding this comment.
rather than get the energy here, pass it in. This would both open up a path to having an extra API of "what is the best combination if I changed to a different energy" without having to actually change the energy.
In both set/read you are in an async function so can do the read and push it down.
| diff = float("inf") | ||
| new_diff = abs(target_transmission - atten) | ||
| inc = 1 if target_transmission > atten else -1 | ||
|
|
There was a problem hiding this comment.
best_inx = np.argmax([_.transmission for _ in available_attenuations])
or np.insert_sorted or https://docs.python.org/3/library/bisect.html
As there are physical attenuator banks we are always going to be in the realm of small numbers and my expectation on the relative runtimes is that computing the transmission is >> more expensive that this search.
To some degree, the sort in _calculate_available_transmissions is a false economy as that is at best O(n * log(n)), but if we just need to find the max we can do that with on O(n) scan (which given that n here is < 256 it it all a bit moot!).
3 participants
Mainly, this PR creates the ability to attenuate a beam by giving control over a set of attenuators:
Also fixed various other issues: