Adds parsing / validation of JSON configuration, for I19 transmission hardware. by CoePaul · Pull Request #2173 · DiamondLightSource/dodal

CoePaul · 2026-08-13T15:10:39Z

Contributes to #2172

Instructions to reviewer on how to test:

Check BaseModel classes against JSON in file mentioned from i19 config in Pydantic BaseModels to capture I19 transmission system JSON specifications #2172 description
Check tests cover reasonable expectations for JSON validation
Ensure CI passes the tests and that the python matches house style ( etc )

N.B. Voids in an i19 absorber wedge are bubbles in the absorber material ( 3-D printer output )
and therefore motor positions to avoid:
Nothing to do with the software void type.

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}

codecov · 2026-08-13T15:17:45Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.35%. Comparing base (e8b29c8) to head (04b22c3).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2173      +/-   ##
==========================================
+ Coverage   99.33%   99.35%   +0.01%     
==========================================
  Files         367      378      +11     
  Lines       14379    14623     +244     
==========================================
+ Hits        14284    14528     +244     
  Misses         95       95

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

DominicOram

Great, I think the main question is around why we have SystemAspectBaseParser when we could use standard pydantic tools instead?

Additionally, are the wedges mounted on the lateral_motors? In which case why are they separate things in the json structure? Why not have something like:

  "wedges": {
    "y": {
      "material" : "aluminium",
      "geometry": {
        "taper_cotangent" : 9.3985,
        "tip" : 5.06,
        "voids": []
        "motion": {
          "units": "mm",
          "out" : 5.0,
          "threshold": 8.9,
          "max": 98.0,
          "tolerance": 5.0e-3
        }
      }
    },
}

This would remove a bunch of edge cases about one not being defined when the other is or the names not matching

I haven't looked at the tests yet, I think once we resolve the discussion on SystemAspectBaseParser they might change a bit anyway.

DominicOram · 2026-09-01T14:50:59Z

+        upper: The upper end of the energy interval (range) for specific absorption fit curve.
+    """
+
+    units: str = Field(..., pattern=r"^(keV|kiloelectronvolts)$")


Should:

Suggested change

units: str = Field(..., pattern=r"^(keV|kiloelectronvolts)$")

units: Literal["keV", "kiloelectronvolts"]

is more concise and readable. It also means the type system will enforce it later if we ever use the value

I didn't know You could enforce a choice of two strings by means of a Literal list...

DominicOram · 2026-09-01T15:00:09Z

+            Optional residuals correction, zeroth order parameter first.
+    """
+
+    photon_absorption: StrictFloat


Could: As above,

Suggested change

photon_absorption: StrictFloat

photon_absorption: StrictFloat= Field(gt=0)

Here it probably makes sense

DominicOram · 2026-09-01T15:09:32Z

+    """
+
+    photon_absorption: StrictFloat
+    roll_off: StrictFloat


Could:

Suggested change

roll_off: StrictFloat

roll_off: StrictFloat = Field(lt=-2, gt=-4)

(probably with -2 and -4 pulled into constants). Let's pydantic do all the checking for you. With this and above you can remove the validate_attributes entirely

a) I wasn't aware Field would work with constants - although it seems reasonable that it can

b) there's no good name for the bounds as they are arbitrary and probably too wide and too "3-centric"
I just don't want +2.67 to be accepted
and accepting -4.2 would imply accepting a typo

but yes it could be better explained - how it is somewhat vague and that vagueness is reflected in the code

They could just be named something like REALISTIC_PHYSICAL_LOWER_BOUND?

DominicOram · 2026-09-01T15:10:47Z

+            f"Absorption roll off {self.roll_off} does not seem likely on physics grounds."
+            if self.roll_off < 0


Should: Why have a separate check on the positive case? Then have a cryptic message that doesn't tell the user how to fix the issue? I think better to remove this case and just check that it's around -3

I'll look again - I'm sure bits of it made sense as I was tweaking it

DominicOram · 2026-09-01T17:18:42Z

+        cls._VALIDATOR.validate_name(name_to_check=axis_name)
+
+
+class MaterialNameValidation:


Could: Why does it matter what someone wants to name their material?

It doesn't - but there's every advantage in picking up typos from edits to JSON.
You can't catch them all - but does that really mean You should catch no categories at all?

I just don't want material 1234_iron. - Why would I allow ints at the start?

Ok, can we at least add a comment to the affect that it's actually just convention/preference? Otherwise I worry that someone will ask for it in 5 years for a legitimate reason and we'll be worried we can't change it as it might break something

DominicOram · 2026-09-01T17:23:55Z

+        - Each absorber wedge has a motor to drive it sideways across the x-ray beam:
+            - expected sideways motions are pure horizontal or pure vertical
+                - but that's a detail, azimuthal orientation around the beam should not matter.
+            - The wedge motor scale has
+                - an **out** position
+                - and then a range of "active" positions, ( where the mathematical assumption of a linear taper is reasonable ).


Should: None of this is actually matched by anything in the Spec so it's not clear why it's here and I need to think about it at this point?

Notes are "You can read this - but You don't have to"

that's why it's a note.
I depends whether the poor developer reading this is trying to understand the whole system or not.

Looking at it from the "I understand the transmission system" end of the telescope - the reader WILL be wondering where's the "move to the absorber OUT the way" gubbins?

Every attenuating element has a "range" of positions which are absorbing
( sometimes - ie for a foil, range = 1 place only )
and a singular location for OUT
( in principle - even if not in practice - lateral motor +/- margin of readout error is roughly a singular location )

OUT is an almost canonical addendum to the "range" of permitted positions for an absorber.

It's a very inhomogenous logical smush together of range + OUT but I couldn't see any way round it

Looking at it from the "I understand the transmission system" end of the telescope - the reader WILL be wondering where's the "move to the absorber OUT the way" gubbins?

Yes, exactly and they're not here. Ok, so happy to leave the description here if we have a note to see LateralMotorSpec?

DominicOram · 2026-09-01T17:27:49Z

+        permissions: Those slot numbers which are permitted to be in use.
+    """
+
+    foils: dict[str, FoilSpec]


Nit: These are always numbers, right?

Suggested change

foils: dict[str, FoilSpec]

foils: dict[int, FoilSpec]

JSON keys are only strings - I didn't invent JSON

so no - they are never integers and always "integers"

Sure, the JSON dict key must be a string but the python dict key doesn't have to be so you could do the conversion here to enforce it to an integer and stop the user putting something in that isn't an integer

DominicOram · 2026-09-01T17:28:32Z

+        raise ValueError(_msg)
+
+
+class WheelsConfig(SystemAspectBaseParser[WheelSpec]):


Must: As above re SystemAspectBaseParser

DominicOram

Sorry, meant the above as a request changes, not a comment

* 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

CoePaul requested a review from a team as a code owner August 13, 2026 15:10

CoePaul added the i19-2 label Aug 13, 2026

CoePaul self-assigned this Aug 13, 2026

CoePaul marked this pull request as draft August 13, 2026 15:11

CoePaul requested review from DominicOram and noemifrisina August 13, 2026 15:11

CoePaul added i19-1 python Pull requests that update Python code labels Aug 13, 2026

CoePaul force-pushed the issue2172 branch from 935ee9b to b70e4b4 Compare August 13, 2026 15:13

CoePaul force-pushed the issue2172 branch 4 times, most recently from 4cf081e to 13c130e Compare August 19, 2026 20:26

CoePaul force-pushed the issue2172 branch from 13c130e to 24074d2 Compare August 27, 2026 15:14

CoePaul changed the title ~~Issue2172~~ Adds parsing / validation of JSON configuration, for I19 transmission hardware. Aug 27, 2026

CoePaul marked this pull request as ready for review August 27, 2026 15:17

CoePaul force-pushed the issue2172 branch 4 times, most recently from 736f148 to 9da488d Compare August 28, 2026 00:05

DominicOram reviewed Sep 1, 2026

View reviewed changes

DominicOram requested changes Sep 1, 2026

View reviewed changes

CoePaul added 2 commits September 2, 2026 14:57

Add transmission system configuration validating reader classes

04b22c3

* Use pydantic BaseModel classes to capture blobs from Transmission System specification config JSON files for I19 beamlines EH-1 / EH-2

CoePaul force-pushed the issue2172 branch from 9da488d to 04b22c3 Compare September 2, 2026 15:14

	units: str = Field(..., pattern=r"^(keV\|kiloelectronvolts)$")
	units: Literal["keV", "kiloelectronvolts"]

	photon_absorption: StrictFloat
	photon_absorption: StrictFloat= Field(gt=0)

	roll_off: StrictFloat
	roll_off: StrictFloat = Field(lt=-2, gt=-4)

		f"Absorption roll off {self.roll_off} does not seem likely on physics grounds."
		if self.roll_off < 0

		cls._VALIDATOR.validate_name(name_to_check=axis_name)


		class MaterialNameValidation:

		raise ValueError(_msg)


		class WheelsConfig(SystemAspectBaseParser[WheelSpec]):

Uh oh!

Conversation

CoePaul commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

CoePaul commented Aug 13, 2026 •

edited

Loading

codecov Bot commented Aug 13, 2026 •

edited

Loading