feat(sc-linac): give LCLS-II-HE cavities their own loaded-Q window - #284
Open
lisazacarias wants to merge 2 commits into
Open
feat(sc-linac): give LCLS-II-HE cavities their own loaded-Q window#284lisazacarias wants to merge 2 commits into
lisazacarias wants to merge 2 commits into
Conversation
Every cavity in L4B (CM 37-59) is an LCLS-II-HE cavity, and those accept a wider loaded-Q window than the original LCLS-II design: 3e7 to 7e7 rather than 2.5e7 to 5.1e7. Wider at both ends, not shifted — a different cavity, not the same one held to a looser standard. Applying the standard window to L4B flags correctly-performing HE cavities as out of tolerance. Source: Ryan Porter's cavity characterization outline, "Flag loaded Q is < 3E7 or > 7E7 (for HE cavities)". Worth his confirmation before treating the numbers as settled — the provenance note in linac_utils says so. Cryomodule.is_high_energy mirrors the existing is_harmonic_linearizer, and the two are mutually exclusive (L1BHL vs L4B), so the limit branches cannot collide; there is a test asserting that holds for every cryomodule. Only the loaded-Q pair is overridden. HE cavities share the standard length, frequency and scale-factor limits, and no separate HE scale-factor values were specified. Landed in the hardware model rather than in the commissioning phase that needs it, so auto setup gets the right limits too — measured_loaded_q_in_tolerance is what both consult. The cavity test fixture now sets is_high_energy explicitly: its cryomodule is a Mock, so an unset property returns a truthy Mock and would have selected the HE window for a fixture whose linac name is always L0B-L3B. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🟢 PR size: 26 lines — within target
Target is 400 lines excluding tests, hard stop around 800 — see |
s-aderhold
reviewed
Aug 24, 2026
…design Sebastian's review correction: the wider HE window comes from a different default Qext, not from the cavities being a different design. Comment and test docstring only — no behaviour change. Rewrapped rather than committed as-is; the suggestion was a single 166-char line, over the 120-char flake8 limit. 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
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
Every cavity in L4B (CM 37–59) is an LCLS-II-HE cavity, and those accept a wider
loaded-Q window than the original LCLS-II design: 3e7 – 7e7 rather than
2.5e7 – 5.1e7. Wider at both ends, not shifted — a different cavity, not the same
one held to a looser standard.
25 changed lines of production code, but please read them with the blast radius
in mind rather than the size:
measured_loaded_q_in_toleranceis consulted byCavity.finish_characterization(), so this changes pass/fail and pushbehaviour for L4B cavities in auto setup as well as in commissioning.
Groundwork for the Cavity Characterization commissioning phase, which needs to
flag loaded Q correctly. Landed here rather than in that phase so both callers
get the same answer.
Operator-visible
Cavity characterization on an L4B cavity now judges loaded Q against 3e7 – 7e7
instead of 2.5e7 – 5.1e7. A measured loaded Q between 5.1e7 and 7e7 on an L4B
cavity previously reported "Loaded Q out of tolerance" and the value was not
pushed to the cavity; it now reports in tolerance and is pushed. A value between
2.5e7 and 3e7 changes the other way — it used to pass and will now be flagged.
This applies to automated setup as well as to commissioning, since both read the
same tolerance check. Nothing changes for L0B–L3B or for the harmonic linearizer
cryomodules (H1, H2).
Scope
25 changed lines excluding tests — within target.
Decisions worth recording
The numbers come from Ryan and should be confirmed by him. Source is his
cavity characterization outline: "Flag loaded Q is < 3E7 or > 7E7 (for HE
cavities)". The provenance note sits on the constants in
linac_utilsso thenext reader knows where they came from and that they were not derived from
anything in the codebase. If they are wrong, this is the one place to fix them.
In the hardware model, not the commissioning phase. The phase that needs
this could have carried its own limits, but
measured_loaded_q_in_tolerancealready exists on
Cavityand auto setup already uses it. Two sources of truthfor the same threshold would mean auto setup keeps flagging HE cavities
incorrectly while commissioning does not — a difference nobody would expect and
which would be found the hard way.
Only the loaded-Q pair is overridden. HE cavities share the standard length,
frequency and scale-factor limits. No separate HE scale-factor values were
specified, so inventing them would have been guessing; if they differ, that is a
follow-up with its own source.
Cryomodule.is_high_energymirrorsis_harmonic_linearizer— same shape,self.name in L4Bagainstself.name in L1BHL. The two are mutually exclusive,so the limit branches cannot both fire; there is a test asserting that holds for
every cryomodule in the machine rather than relying on it being obvious.
Learning reviewer
@hmarts9 —
Cavity.__init__is where a cavity's class-specific limits getresolved, and
finish_characterization()is what consumes them. Worth a read tosee how the model distinguishes cavity types. No approval needed.
Testing
flake8clean including--max-complexity=10;blackclean; pre-commit cleanmain; no overlap with feat(rf-commissioning): add frequency tuning UI #270 or feat(rf-commissioning): implement the frequency tuning phase backend #282 (verified byintersecting their changed-file sets)
Checked against the real machine rather than only through mocks:
There is also a test for the behavioural consequence directly: a measured loaded
Q of 6e7 passes
measured_loaded_q_in_toleranceon CM37 and fails on CM01.Two things worth knowing about the test changes:
test_loaded_q_limitsneeded updating. Its fixture's cryomodule is aMock,so the new
is_high_energyproperty returned a truthy Mock and selected the HEwindow. The fixture sets
linac.nameto L0B–L3B, so its intent is plainlynon-HE; it now sets
is_high_energyexplicitly.CM 36 does not exist — L3B ends at 35 and L4B starts at 37. My first
version of the test asserted on it and raised
KeyError. Now noted in the test.pytestpassesCoverage still clears 80%
Checked against simulation (
PYDM_DEFAULT_PROTOCOL=fake/sc-sim) where applicable