fix(qe): reject unresolved cutoffs and close silent under-convergence paths - #14
Merged
Conversation
…g to 0.0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp3mfWC9io4qwRsWuXM1Ec
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp3mfWC9io4qwRsWuXM1Ec
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp3mfWC9io4qwRsWuXM1Ec
When the user supplies one of ecutwfc/ecutrho and leaves the other empty, require only the empty field from the pseudopotential source instead of both, so a valid input is no longer rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp3mfWC9io4qwRsWuXM1Ec
…lling the other Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kp3mfWC9io4qwRsWuXM1Ec
Review of the qe-cutoff-required work (issue #13) found paths that still let unresolved cutoffs slip into generated inputs: - treat blank cutoff-CSV cells (pandas NaN) and non-positive UPF header cutoffs (ld1.x writes wfc_cutoff="0.0") as "not specified" - fill only the missing field from the UPF header instead of discarding a CSV value when the other field is missing - fill ecutwfc also when the template omits the key entirely; pw.x requires it, so a template without it silently produced invalid input - stop hard-erroring on unresolved ecutrho: omit the key and let pw.x default to 4*ecutwfc (it is optional in QE and normally absent for norm-conserving sets); in mixed sets the suggested values compete against 4*ecutwfc of the elements without a suggestion - point the error remedy at content.namelist.system.ecutwfc, the real YAML path (content.system.* would be treated as a card) - drop the tautological guard in _find_elem_cutoff and dedupe the test setup into _qe/_write_upf/_pw_mode helpers Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt paths Second review round on #13 found paths where the cutoff rework traded hard errors for silently under-converged inputs: - floor an explicitly written ecutrho at 4 * the ecutwfc actually emitted (resolved max, or the user-supplied value), so it never falls below the default pw.x would have used; tolerate rounding-level differences so CSVs storing a rounded 4*ecutwfc keep their value - when the template carries no cutoff keys at all, resolve ecutrho too: filling only ecutwfc left ultrasoft sets on the 4*ecutwfc default instead of the suggested ~8-12x - fill only header-missing fields from the pp_info "Suggested" lines; an unset 0.0 line could reset a valid header wfc_cutoff to None and turn a resolvable pseudopotential into a hard error - warn on unparseable cutoff values and on elements excluded from the ecutrho competition (both were silent) - reject pw.x tasks without a &system namelist instead of writing an input that pw.x cannot run - consult the UPF file only when a needed field is unresolved, and log an open failure as warning: successful runs no longer emit ERROR noise - align utils/pp_cutoff.py with the runtime convention: normalize 0.0/unparseable header values and emit a blank CSV cell (instead of dropping the row) when only rho_cutoff is missing Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
第2回レビュー(issue #13 のコメント参照)で確認された残課題を b5771ae で修正しました:
テスト: 261 passed。NaCl サンプルで実機確認済み(参照一致・ecutwfc=100 明示時に ecutrho=400・キー無しテンプレートで両方補完・&system 無しで exit 1)。 🤖 Generated with Claude Code |
…lled wheel src/utils/pp_cutoff.py is a standalone helper, not part of the installed distribution: importing it as `utils.pp_cutoff` only worked when conftest put src/ on sys.path, and broke CI's CIF2X_TEST_INSTALLED run. Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Third review round (Codex, strict research-software reviewer; 3 rounds to convergence, must_fix now empty): - reject a None or non-mapping &system section (a bare "system:" in YAML parses to None) with InputValidationError instead of crashing with TypeError in the update helpers; same for content.namelist=None, which Content() uses as its default - validate a user-supplied ecutwfc before the 4*ecutwfc floor: reject non-numeric values with a clear error, accept numeric strings leniently and write the normalized float back so f90nml does not render a quoted Fortran string - regression tests for all three paths, plus a pp_cutoff guard for an absent rho_cutoff attribute yielding a blank CSV cell Refs #13 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Third review pass, this time with Codex as an independent strict reviewer (different angle from the two multi-agent correctness reviews). Three rounds to convergence; fixes landed in 329a7e7:
Round 3 reported no remaining must_fix. Tests: 266 passed. |
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.
Summary
Make the QE input generator handle unresolvable cutoffs (ecutwfc/ecutrho) explicitly instead of silently writing 0.0 into the generated input.
The first commits (ba4b6c2, 70bbec9, f0595f0) implement the rejection of unresolved cutoffs; the follow-up commits (0bfeeef, b5771ae) fix the gaps found by two code-review rounds tracked in #13.
Closes #13
Changes
Reject unresolved cutoffs (ba4b6c2 ...)
_find_elem_cutoffraisesInputValidationErrorinstead of falling back to 0.0 when neither the cutoff CSV nor the UPF header resolves a cutoff.content:are resolved (need_wfc/need_rho).First review round (0bfeeef, #13)
_normalize_cutoff). Previously they slipped past theis Nonechecks andecutrho = NaN/ecutwfc = 0.0ended up in the generated input.ecutwfcalso when the template omits the key entirely: it is mandatory for pw.x, and the input used to be written without it.ecutrhowhen it cannot be resolved and let pw.x apply its own default (4*ecutwfc). It is optional in QE and normally absent for norm-conserving sets, so onlyecutwfcremains a hard error.content.namelist.system.ecutwfc(content.system.*would be treated as a card).Second review round (b5771ae, #13 comment)
ecutrhoat 4 * the ecutwfc actually emitted (the resolved maximum, or the user-supplied value), so it never falls below the default pw.x would have chosen; rounding-level differences are tolerated so CSVs storing a rounded 4*ecutwfc keep their value.ecutrhotoo: filling onlyecutwfcleft ultrasoft sets running on the 4*ecutwfc default instead of the suggested ~8-12x.wfc_cutoffto None and turn a resolvable pseudopotential into a hard error.&systemnamelist instead of writing an input pw.x cannot run.utils/pp_cutoff.pywith the runtime convention: normalize 0.0/unparseable header values and emit a blank CSV cell (instead of dropping the row) when onlyrho_cutoffis missing.Verification
pytest tests/→ 261 passed (all fixes were implemented test-first).sample/cif2x/qe/NaCl:scf.in_refecutrholine is omitted,ecutwfckeeps the CSV valueecutwfc:: filled from the CSV; withecutwfc: 100supplied,ecutrho = 400(the 4*ecutwfc floor)&system: rejected with exit 1Note: the QE test modules skip silently when
qe_toolsis unavailable; running them locally requiresqe-tools<2(2.x removedqe_tools.parsers.qeinputparser; pyproject pins^1.1).🤖 Generated with Claude Code