Take the cutoffs from the input when the pseudo family has none

[elinsc-bot]

Problem

A pseudopotential family that recommends no cutoffs could not be used, and the failure spoke the wrong language. koopmans' own SG15 installer produces such a family, as does aiida-pseudo install family. A user who installed SG15 through koopmans and then ran with it met:

ValueError: failed to obtain recommended cutoffs for pseudo family
            `CutoffsPseudoPotentialFamily<SG15/1.2/PBE/FR>`: no default
            stringency has been defined.

Every noun in that is aiida-quantumespresso's — stringency, the aiida-pseudo class name, overrides — and it names a knob the input file does not expose.

Separately, cutoffs stated in the input file were half-taken or silently dropped:

• pw.system.ecutwfc: 45 against a family recommending 17.6 Ry built a calculation running 45 Ry against 17.6 Ry — a ratio of 0.4, a pair neither the input nor the family asked for.
• ecutwfc: 20 at the top level with pw.system.ecutwfc: 45 ran pw.x at 45 Ry and kcp.x at 20 Ry, so the two codes saw different grids.
• ecutwfc: 20 at the top level with kcp.system.ecutwfc: 30 ran both codes at 20 Ry, discarding the stated 30 — kcp reads the shorthand first and its own block only as a fallback, the opposite precedence to pw.

Changes

• The cutoffs come from the input file when the family recommends none. calculator_parameters.ecutwfc alone is a complete statement: ecutrho follows at four times it, which for the norm-conserving pseudopotentials koopmans is confined to is not an independent quantity — QE documents 4x as the default, and all 286 pseudos across the six families installed here publish exactly that.

• An input naming no cutoffs against such a family is refused up front, in koopmans' vocabulary rather than the plugin's, naming calculator_parameters.ecutwfc.

• That check sits at three call sites, not one: the DSCF and wannierize-blocks routes build their own scf/nscf overrides instead of going through prepare_common_inputs, so a single check would leave both uncovered.

• ecutrho stated on its own raises, naming ecutwfc. Inverting the ratio is arithmetic, not intent; an input naming only the density cutoff has left the other out by mistake.

• Any pair off the ratio of four takes effect with a warning naming the key it came from. Nothing is half-taken, so there is nothing to refuse — the warning is what says the pair is not a norm-conserving one.

• Every stated cutoff must agree. Values that disagree across calculator_parameters.ecutwfc, pw.system and kcp.system are rejected at parse rather than sending the two codes to different grids.

• A cutoff of zero or less is rejected at parse. It previously reached a division and produced a bare ZeroDivisionError; a negative one passed through entirely.

• CI and Read the Docs now clone aiida-quantumespresso from our fork's patched branch, which carries the upstream fix (👌 Protocols: support pseudo families without recommended cutoffs aiidateam/aiida-quantumespresso#1281) that lets such a family reach a builder at all.

Testing

• pw.system.ecutwfc: 45 against a family recommending 17.6 Ry builds at 45 / 180. Asserting the derived value is what discriminates: a test that only checked the build no longer raises would pass against code silently keeping 17.6.

• The up-front refusal is pinned on both routes that carry their own check, and the two fail differently when it is removed. On wannierize-blocks the message reverts to aiida-quantumespresso's stringency text, since that route builds the pw protocol eagerly. On DSCF nothing raises at all — the graph builds and carries no wavefunction cutoff into its pw.x steps. Silence, not a worse message, which is why that one needed its own test.

• Each rejection has a negative control: with the rule reverted, every raise-case reports DID NOT RAISE, and the accept-cases still pass.

• The off-ratio warning is checked outside pytest against real stderr, and a second test pins that an on-ratio value stays silent, so the first cannot pass by warning on everything.

• All four tutorial inputs stay legal under the agreement rule.

Note for review

This branch depends on aiida-quantumespresso patched at d800109. Until aiidateam merges #1281, k2 main is red if that branch moves.

[read-the-docs-community]

Documentation build overview

📚 koopmans-dev | 🛠️ Build #33963399 | 📁 Comparing a3035ef against latest (81be694)

  🔍 Preview build  

5 files changed · ± 5 modified

± Modified

• cli.html
• genindex.html
• input_schema.html
• _modules/koopmans/input_file.html
• _modules/koopmans/input_file/pw.html

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.87%. Comparing base (3689767) to head (a3035ef).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   79.29%   81.87%   +2.57%     
==========================================
  Files          44       44              
  Lines        3381     3464      +83     
  Branches      526      543      +17     
==========================================
+ Hits         2681     2836     +155     
+ Misses        566      493      -73     
- Partials      134      135       +1     

☔ 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.

[elinscott]

Why do we need to explicitly provide pseudos in this case? Why doesn't the pseudo_family work any more?

[elinsc-bot]

The family still supplies the pseudos — get_pseudos_from_family(pseudo_family, structure) is where they come from either way. What changes is who resolves it, and it is a workaround for upstream rather than something koopmans wants.

Handed a pseudo_family name, get_builder_from_protocol looks up the family's recommended cutoffs before it gets anywhere near the pseudos, and raises when the family has no stringency defined:

ValueError: failed to obtain recommended cutoffs for pseudo family
            `CutoffsPseudoPotentialFamily<SG15/1.2/PBE/SR>`: no default
            stringency has been defined.

There is no way to tell it "use this family, take the cutoffs from me". But it has a second branch: given explicit pseudos, it skips the cutoff lookup entirely. So resolving the family ourselves one step early takes that branch and dodges a lookup whose result we were going to overwrite anyway.

Two things worth knowing:

• It only fires for a family that publishes no cutoffs (pseudo_family_has_cutoffs returns early otherwise), so the normal path is untouched.
• It has a visible cost: aiida-quantumespresso then warns Found unrecognised key in overrides: scf.pw.pseudos.Si on every affected route, about the key its own builder reads. Noise, but a user sees it — flagged in the PR body as worth reporting upstream.

Graded code-read, not reproduced: I have read upstream's two branches, and the tests pin that the pseudos arrive and the cutoffs are the input's. I have not separately reproduced the claim that the explicit-pseudos branch is why the lookup is skipped.

[elinscott]

This is far too complex: Should just read: "ecutrho should be 4 x ecutwfc for nomr-conserving pseudos."

[elinsc-bot]

Cut to the rule. It now reads:

`calculator_parameters.pw.system.ecutrho` = 300 Ry: ecutrho should be 4 x ecutwfc
= 180 Ry for norm-conserving pseudopotentials. Drop
`calculator_parameters.pw.system.ecutrho` to take that.

Gone: the sentence explaining that the two converge together. Kept the value it should have been (180 Ry) and the key to drop, so the reader does not have to do the arithmetic or hunt for which key to edit.

04dc886.

[elinscott]

Just approved, but would probably rather wait till upstream aiida-quantumespresso issue is fixed and dropping the extra guards before merging as they introduce incorrect warnings.

[elinscott]

Why doesn't it call prepare_common_inputs?

[elinsc-bot]

Because prepare_common_inputs builds a shape this route does not use, and resolves things it has already resolved.

It returns (structure, pseudo_family, overrides) where overrides is flat — one entry per sub-workflow key, {"scf": {...}, "nscf": {...}} — built for a route that hands the whole thing to one graph builder. dscf_wannier_init_inputs is a helper, called with structure and nbnd already in hand from its caller, and what it assembles is a WannierizeOverrides feeding a per-block fan-out. Calling prepare_common_inputs would redo the structure conversion and the family install, then discard most of what came back.

Worth saying plainly though: three call sites for one check is a wart, and you are right to poke at it. The map today is

module	prepare_common_inputs	own cutoff check
dft, eps, dfpt	yes	via that
wannierize (whole-manifold)	yes	via that
wannierize (block-by-block)	no	its own
dscf, trajectory	no	its own

So it is not one route being awkward — it is that "prepare the shared pw inputs" and "validate the family against the input" got bundled into one function, and only half the routes want the first. Splitting the validation out, so every route calls it once regardless of how it builds its overrides, would be the real fix. I did not do it here because it touches every route's entry point and this PR was already large; happy to open an issue.

[elinscott]

Again, why?

[elinsc-bot]

Same root cause, different reason for the split — and this one is a branch rather than a helper.

build_wannierize_workgraph returns early to _build_wannierize_blocks_workgraph at lines 189 and 193, and only reaches prepare_common_inputs at line 197. So the whole-manifold route uses it and the block-by-block route never gets there. The block route needs per-step overrides that feed a fan-out, not the flat one-entry-per-key shape.

Worth flagging that this is exactly where the risk was: I originally briefed this as "only DSCF needs its own check, because wannierize.py calls prepare_common_inputs". It does — on the other branch. Following that brief would have left the block route silently uncovered, and it was caught by writing the negative control rather than by reading.

The two also fail differently when the check is removed, which is why each has its own test: here you get aiida-quantumespresso's stringency message back, because this route builds the pw protocol eagerly. On DSCF you get nothing at all — the graph builds and carries no wavefunction cutoff into its pw.x steps.

Fix for the duplication is the same one as on the DSCF thread: split the validation out of prepare_common_inputs so every route calls it once, however it builds its overrides.