Handle nscf outputs that lack the scf-Fermi stdout marker - #92
Open
elinscott wants to merge 7 commits into
Open
Conversation
QE >= 7.6-dev no longer prints the "(compare with: ... computed in scf)" line in nscf runs, so get_fermi_energy_from_nscf silently returned None, which then crashed deep inside the .win writer with an opaque "Invalid value" error. Fall back to the Fermi energy the parser extracted from the nscf output itself, and raise loudly in prepare_wannier90_pp_inputs if the value still resolves to None. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Spin-polarised runs with a constrained total magnetization report fermi_energy_up / fermi_energy_down instead of a single fermi_energy; take the highest of the two (the electron chemical potential across both channels). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The nscf fallback read output_parameters["fermi_energy"] with no units check, unlike get_fermi_energy which returns None unless the units are eV; add the same units guard so a non-eV value cannot be returned silently. Reword the fallback's docstring/comments: the stdout marker is governed by the nscf occupation/spin type (QE prints the single-value "compare with" line only for a metallic, single-Fermi-energy nscf), not by QE version, and max(up, down) is a conservative frozen-window reference, not a chemical potential. Also guard get_workchain_fermi_energy in plot/bands.py: its nscf branches consume the same helper and could return None into the band-plotting arithmetic, mirroring the guard already added in prepare_wannier90_pp_inputs.
Cover marker-present (scf value used), marker-absent fallback to the parsed nscf Fermi energy, the spin-polarised max(up, down) branch, the None-when-unavailable case, and the non-eV units guard. Pure-function tests with stubbed nodes; not executed under pytest in this environment because the suite's autouse fixture requires a database profile.
test_scdm mocked the scf output_parameters with fermi_energy but no fermi_energy_units, so get_fermi_energy returned None and the new guard raised "Fermi energy resolved to None". Add the units key, and assert that the Fermi energy reaches the wannier90 pp parameters, which the test previously left unchecked. Also reword both guards: they named the nscf workchain and the stdout marker even when the scf branch produced the None, and said nothing about what to change. They now name the workchain actually consulted and the output_parameters keys it must carry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Pushed: the mock gains its units, and the test now asserts the Fermi energy actually reaches |
Resolve the conflict in `prepare_wannier90_pp_inputs`: keep both main's "a caller-supplied `fermi_energy` wins over reading the nscf" shortcut and this branch's `fermi_source` record of which workchain supplied the energy. - Fold main's shortcut together with the pre-existing "no workchain at all" branch, which does the same thing under a different guard. - Set `fermi_source` to None when the energy came from the caller's parameters rather than from a workchain, and give that case its own guard message.
`prepare_wannier90_pp_inputs` picks the Fermi energy from the nscf run in the context or from the caller's wannier90 parameters, and no test drove either path: the existing `test_scdm` always has an scf run in context. - Read from the nscf run when the parameters carry no `fermi_energy`. - Keep the caller's `fermi_energy` when the parameters carry one, even though an nscf run is in the context. - Reject a `fermi_energy` of None in the parameters, with a message that names the parameters rather than the nscf run.
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.
Problem
get_fermi_energy_from_nscfextracts the scf Fermi energy from an nscf run by matching the stdout line(compare with: X eV, computed in scf). That companion line is not always present, so the helper silently returnedNone. TheNonethen flowed throughprepare_wannier90_pp_inputsinto the wannier90 input parameters, where the.winwriter finally failed with an opaqueInvalid value passed, accepts only bools, ints, floats and strings.As per the QE source (
PW/src/print_ks_energies.f90,print_ks_ef_homolumo), the single-value marker the regex matches is emitted only for a metallic (smearing/tetrahedra), single-Fermi-energy nscf run: insulators print HOMO/LUMO instead, and constrained-magnetization (tot_magnetization) runs print a two-value variant that the regex does not match. This is governed by the occupation/spin type of the nscf run, not by the QE version — theprint_ef_scfguard is unconditionally true for nscf runs, identically in the 7.4.1, 7.5 and 7.6-dev sources I checked.Changes
get_fermi_energy_from_nscffalls back to the Fermi energy the parser extracted into the nscf'soutput_parameterswhen the stdout marker is absent, and its return type annotation is corrected toOptional[float](it could already returnNoneon a failed match). The fallback also handles constrained-magnetization runs, which report one Fermi level per channel, by takingmax(fermi_up, fermi_down)as a conservative reference for the frozen windows.fermi_energy_units == "eV", mirroringget_fermi_energy, so a value in the wrong unit cannot be returned silently (the aiida-quantumespresso parser always stores eV, so this is a latent-trap guard rather than a behavioural change).prepare_wannier90_pp_inputsraises a descriptiveValueErrorif the Fermi energy still resolves toNone, instead of passing it through to the.winwriter. The plotting helperget_workchain_fermi_energy(utils/workflows/plot/bands.py), whose nscf branches consume the same value, gains the same guard.get_fermi_energy_from_nscfcover the marker-present path, the fallback to the parsed nscf Fermi energy, the spin-polarisedmax(up, down)branch, theNone-when-nothing-available case, and the non-eV units guard.Notes
The stdout marker remains the first choice where present, preserving current behaviour where the current machinery works (the marker reports the scf Fermi energy, which can differ from the nscf one).
Design question
Shouldn't this Fermi energy come from the nscf in the first place — always, not just as a fallback? The value feeds
shift_energy_windowsapplied against the nscf eigenvalues, so surely its fermi level should be our reference? It will also typically be a more accurate reference, evaluated with better Brillouin-zone sampling cf. the scf.However, using the nscf fermi level by default would be a breaking change, shifting the reference for every user rather than just skip-scf runs (cf. this PR, which keeps behaviour unchanged where the current machinery works.)
Testing
Exercised end-to-end against pw.x 7.6-dev nscf output: previously the wannier90 calculation excepted in presubmit; with the fallback the pp run receives the parsed Fermi energy and completes.