feat(rf-commissioning): implement the cavity characterization phase backend - #285
Open
lisazacarias wants to merge 3 commits into
Open
feat(rf-commissioning): implement the cavity characterization phase backend#285lisazacarias wants to merge 3 commits into
lisazacarias wants to merge 3 commits into
Conversation
…ackend Probe calibration and loaded-Q measurement for the CAVITY_CHAR commissioning phase, following Ryan Porter's outline. No UI — the existing placeholder display covers CAVITY_CHAR, so this can land and be exercised on its own. Deliberately thin. Cavity already implements the measurement: start_characterization(), the characterization_running / _crashed predicates, calculate_probe_q(), push_loaded_q(), push_scale_factor(), and the per-cavity-class tolerance check measured_loaded_q_in_tolerance. Auto setup's request_characterization() is a twelve-line wrapper over the same code and this phase is a similarly thin one; the module docstring says so, so that a step body growing past a few lines of orchestration reads as a warning sign. One deliberate divergence from Cavity.characterize(): that bundles start, wait and push into a single blocking call, which suits unattended setup. Commissioning needs the operator to see loaded Q, scale factor and probe Q before anything is written to the cavity, so read_results and push_results are separate steps and nothing reaches the hardware until the operator confirms. An out-of-tolerance loaded Q is flagged, not blocked — whether to push it is their call. CavityCharacterization gains loaded_q_in_tolerance, and passed now requires it. Previously passed was just is_complete, so a characterization that measured an out-of-range loaded Q recorded as passed — which would have made Ryan's "flag loaded Q" requirement cosmetic. Written as "is not False" so records predating the field keep their previous result instead of retroactively failing. Two items from the outline are deliberately left open rather than guessed: - Drive level. The outline asks for 10% on 7 kW SSAs and 15% on 3.8 kW ones, but there is no SSA power rating in the hardware model — no rated-power PV, and fwd_power_lower_limit is a calibration floor, not a rating. The phase starts from the existing safe pulsed level and accepts an operator override, which the outline asks for anyway. Needs the SSA question answered. - The characterization plot. CAV/FWD/REV:AWF waveform PVs already exist on Cavity; drawing them belongs with the UI work. Prerequisite recency (SSA calibration and tuning) is a warning, not a gate: the workflow already enforces that those phases ran, and re-driving hardware from here is not this phase's decision. It reads their timestamps off the record. Step results are stored on the record as each step produces them, the same pattern as the tuning phase and for the same reason — phase_history is in-memory only and does not survive a restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🟡 PR size: 503 lines — over the 400-line target
Target is 400 lines excluding tests, hard stop around 800 — see |
_read_probe_q() looked for cavity.probe_q / cavity.measured_probe_q. Neither exists. QPROBE_CALC1.PROC triggers the calculation and is the only probe-Q PV in the codebase — there is no value record to read the result from, and no PUSH_QPROBE either; only PUSH_QLOADED and PUSH_CAV_SCALE exist. So probe Q was always going to be None on real hardware while the code read as though it were measured. It now returns None explicitly, with the gap documented in the module docstring and at the call site. The test suite hid this. The mock cavity set probe_q = 2.0e9, so the assertion that the record carried a probe Q passed against an API that does not exist. The fixture no longer invents the attribute, and two tests pin the gap instead: one that the record leaves probe_q empty, and one asserting the real Cavity has no probe-Q readback — which will fail, deliberately, the day the PV is added, so whoever adds it is pointed straight at the code to wire up. Ryan's outline asks for probe Q to be displayed and pushed, so those PVs are presumably known to the SRF group and simply absent from the hardware model. Better an empty field on the record than a fabricated number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Implements the backend for the Cavity Characterization commissioning phase: probe
calibration and loaded-Q measurement, following Ryan's outline. No UI — the
existing placeholder display already covers
CAVITY_CHAR, so this lands and canbe exercised on its own.
Read
phases/cavity_char.pytop to bottom; the module docstring states what isdelegated and why, and the step bodies are meant to be short enough to scan.
Operator-visible
Not yet, in practice.
CAVITY_CHARstill shows the generic placeholder display,so nothing new appears on screen until the phase UI is built. The behaviour this
adds becomes visible then.
One change does reach existing records: a characterization whose loaded Q falls
outside the expected window for that cavity is now recorded as complete but not
passed. Previously any characterization that produced numbers was recorded as
passed regardless of the value.
Scope
493 changed lines excluding tests — over the 400 target.
474 of those are the single new module
phases/cavity_char.py. The remaining ~19are the
loaded_q_in_tolerancefield it needs and one export line.Why it is not split further: the file is one phase's seven-step sequence, and the
steps share the measured state they hand between each other. Splitting it would
mean landing a phase that measures but cannot record, or records but cannot
measure — a half-phase that the workflow would happily let an operator start.
The alternative shape, landing the module with some steps stubbed, moves the same
review burden into a second pass without reducing it.
This is one of a series.
#284(25 lines) went first because theloaded-Q window it fixes is what this phase flags against.
Decisions worth recording
Wrap
Cavity, notSetupCavity.Cavityalready implements the wholemeasurement:
start_characterization(), thecharacterization_running/characterization_crashedpredicates,calculate_probe_q(),push_loaded_q(),push_scale_factor(), and the per-cavity-classmeasured_loaded_q_in_tolerance.Auto setup's
request_characterization()is a twelve-line wrapper over the samecode. Going through
SetupCavityinstead would drag in request-flag PVs and itsprogress reporting, neither of which belong in a gated, operator-driven phase —
so this wraps
Cavityat the same level auto setup does. The module docstringsays the phase is meant to stay thin, so that a step body growing past a few
lines of orchestration reads as a warning that something is being duplicated.
Read and push are separate steps.
Cavity.characterize()bundles start, waitand push into one blocking call, and
finish_characterization()pushes loaded Qand scale factor automatically when they are in tolerance. That is right for
unattended setup and wrong here: Ryan's outline asks that the operator be able to
push and save the values, which means seeing them first. So the phase drives the
same shared code at a lower level and stops between measuring and pushing.
read_resultstouches nothing on the cavity — there is a test asserting the pushmethods are never called during it.
Out of tolerance is flagged, not blocked. The measurement succeeded and the
operator needs to see it; whether to push a marginal loaded Q is their judgment.
The record carries the flag either way.
passednow requires the tolerance flag.CavityCharacterization.passedwasis_complete, so a characterization that measured an out-of-range loaded Qrecorded as passed — which would have made "flag loaded Q" cosmetic. The new
loaded_q_in_tolerancefield is recorded rather than recomputed because thelimits live on the
Cavityobject and a record has no access to one.passediswritten as
is not Falseso records predating the field keep their previousresult instead of retroactively failing.
Prerequisite recency warns rather than gates. Characterizing a freshly
calibrated and freshly tuned cavity is best practice, but the workflow already
enforces that
SSA_CHARandFREQUENCY_TUNINGran, and re-driving hardware isnot this phase's call. It reads their timestamps off the record and notes staleness
in the step message.
Two of Ryan's items are left open rather than guessed:
There is no SSA power rating anywhere in the hardware model — no rated-power PV,
and
fwd_power_lower_limit(3000 / 500) is a calibration floor, not a rating.Guessing a class from it would set drive on a cold cavity from an inference. The
phase starts from the existing
SAFE_PULSED_DRIVE_LEVELand accepts an operatoroverride, which the outline wants regardless. The reasoning is recorded on
CavityCharLimits. Needs an answer: is there a PV that reports SSA power, oris the split known from the cryomodule?
CAV:AWF,FWD:AWFandREV:AWFalready exist onCavity; drawing them belongs with the phase UI.QPROBE_CALC1.PROCtriggers thecalculation and is the only probe-Q PV in the codebase — there is no value
record to read the result from, and no
PUSH_QPROBE(onlyPUSH_QLOADEDandPUSH_CAV_SCALEexist). The phase triggers the calculation and leavesprobe_qempty on the record rather than fabricating a number. The outlineasks for probe Q to be displayed and pushed, so those PV names are presumably
known to the SRF group and simply missing from the hardware model — what are
they? A test asserts the readback is absent, so it will fail the day the PV
is added and point at the code to wire up.
Learning reviewer
@hmarts9 —
read_resultsandpush_resultsare where this phase deliberatelydiffers from
Cavity.characterize(). Worth checking that the review-then-pushsplit matches how you would actually want to work through a characterization, and
whether anything you rely on is missing from what gets recorded. No approval
needed.
Testing
phases/cavity_char.pyat 88%flake8clean including--max-complexity=10;blackclean; pre-commit cleanmain; verified no file overlap with feat(rf-commissioning): add frequency tuning UI #270, feat(rf-commissioning): implement the frequency tuning phase backend #282, or theHE limits PR by intersecting their changed-file sets
32 tests, weighted toward delegation and boundaries rather than arithmetic, since
the arithmetic lives in
Cavity:start_characterization,calculate_probe_q, the tolerance check)read_results, and pushing before measuring failsNone) is refused and thecavity's drive is left untouched — this writes to a cold cavity
finishes
dry_runtouches no hardwarepinned by a test against the real
CavityWorth a reviewer's own check: drive the phase against
sc-simand confirm theloaded Q and scale factor read back as expected. Probe Q will be empty — see the
open item above; that is the behaviour, not a bug in the run.
pytestpassesPYDM_DEFAULT_PROTOCOL=fake/sc-sim) where applicable