Conversation
* Start with adding JSON validation for system specification JSON dict-like structure expectations * Pydantic BaseModel classes map sub-structures ( sub-dicts ) within the system description structure here adding JSON validation for energy_interval [ the range of x-ray energies (in keV ) over which an absorption curve is valid ] JSON validation for absorption fit curve parameters [ the scaling constant, roll-off vs energy and an optional residuals polynomial ]
* Use pydantic BaseModel classes to capture blobs from Transmission System specification config JSON files for I19 beamlines EH-1 / EH-2
| def get_motor_position_mm(self) -> float: ... | ||
|
|
||
|
|
||
| class AbsorberWedge(AbsorptionContributer): |
There was a problem hiding this comment.
Nit: Having two things called AbsorverWedge and WedgeAbsorber is confusing
There was a problem hiding this comment.
Yeah I know - I will have to rationalise the names -
each class is allegedly taking on some unique responsibility
but there may well be scope for deleting a lot of it later when the duplications of function become more obvious
The fact / bad smell that the name is nearly a duplicate - and would be an exact duplicate if I hadn't deliberately avoided a name clash - is definitely a clue that things are not yet in a good state
Some things are over in the "maths library" and others are in the i19 specific transmission system gubbins ( and the alleged reason for them being here is to participate in the "business logic" )
my current dilemma is to not put i19 specific aspects onto the general maths wedge
still trashing it out - as You can tell
| def __init__( | ||
| self, | ||
| *, | ||
| axis_label: str, | ||
| motor_position_reader: ReadableLinearMotor, | ||
| system_config: SystemConfiguration, | ||
| ): | ||
| wedge_spec: WedgeSpec = WedgesConfig.extract_wedge_specifications( | ||
| system_configuration=system_config, wedge_identifier=axis_label | ||
| ) | ||
| self.motor_reader: ReadableLinearMotor = motor_position_reader | ||
| self.scale: LateralMotorSpec = ( | ||
| LateralMotorsConfig.extract_motors_specifications( | ||
| system_configuration=system_config, motor_identifier=axis_label | ||
| ) | ||
| ) | ||
| absorber_material_name: str = wedge_spec.material | ||
| absorption_spec = ( | ||
| MaterialAbsorptionSpectralConfig.extract_absorber_material_specifications( | ||
| system_configuration=system_config, material_name=absorber_material_name | ||
| ) | ||
| ) | ||
| absorption_spectrum: MaterialAbsorptionSpectrum = absorption_spec.as_spectrum() | ||
| tapered_geometry: TaperedGeometryProvider = WedgeGeometry( | ||
| taper_cotangent=wedge_spec.geometry.taper_cotangent, | ||
| tip_mm=wedge_spec.geometry.tip, | ||
| ) | ||
| self.absorber: Final[VariableDepth] = WedgeAbsorber( | ||
| spectrum=absorption_spectrum, | ||
| geometry_model=tapered_geometry, | ||
| ) | ||
| self.id: str = axis_label | ||
| self.voids: list[OpenInterval] = wedge_spec.geometry.voids | ||
| self.absent_when_out = AbsenceFromBeam() |
There was a problem hiding this comment.
If you remove SystemConfiguration, as previously suggested, I would suggest this become something more like:
def __init__(
self,
*,
axis_label: str,
motor_position_reader: ReadableLinearMotor,
full_spec: TransmissionSystemSpec,
):
wedge_spec: WedgeSpec = full_spec.wedges[axis_label]
self.motor_reader: ReadableLinearMotor = motor_position_reader
self.scale: LateralMotorSpec = full_spec.lateral_motors[axis_label]
absorber_material_name: str = wedge_spec.material
absorption_spec = full_spec.materials[absorber_material_name]
absorption_spectrum: MaterialAbsorptionSpectrum = absorption_spec.as_spectrum()
...which is a lot more concise and understandable IMO
There was a problem hiding this comment.
Indeed You are correct
| ) | ||
|
|
||
|
|
||
| class ReadableLinearMotor(Protocol): |
There was a problem hiding this comment.
Is this protocol just a placeholder? I feel like we can just use the concrete linear motor?
There was a problem hiding this comment.
placeholder / strut thing - it will become the linear motor RO when that exists
although the joy of using the Protocol here is that if we leave it in place - we can use anything later
on the calling side and it won't matter
| xray_energy_kev=xray_energy_kev, motor_position_mm=motor_position_mm | ||
| ) | ||
|
|
||
| def is_feasible(self, *, xray_energy_kev: float, demand_bn: float) -> bool: |
There was a problem hiding this comment.
Nit: I feel like this could reuse more of the parent's overridden method? E.g. you could just override _absorption_exceeds_demand with _threshold_absorption_exceeds_demand and then you won't need to repeat is_feasible here?
There was a problem hiding this comment.
I get that impression here or in similar places a lot
( I don't instantly remember where I was when I had that sense, nor exactly where this is )
but I did try things in the spirit You suggest.
In a more mature form - such things might reduce to that form - but I found there were more
child specific things than I could just discount -
in other words - this ( or at least similar ) seemingly superfluous duplication has been done
consciously - even if my reasons might not stand up to more exacting scrutiny
2 participants
Work in progress - To be written
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}