-
-
Notifications
You must be signed in to change notification settings - Fork 0
Take the cutoffs from the input when the pseudo family has none #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8c05443
b8939c2
b1ab107
419d6f0
e2660df
1a87738
354cd90
ecc12c9
cd50eb7
555b7b5
04dc886
5eab827
6ab63af
93444cb
a3035ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,11 @@ | |
| from aiida_quantumespresso.common.types import SpinType | ||
|
|
||
| from koopmans.aiida.conversion import atoms_input_to_structure, input_to_pw_parameters | ||
| from koopmans.aiida.workflows import pin_step_kpoints, prepare_common_inputs | ||
| from koopmans.aiida.workflows import ( | ||
| pin_step_kpoints, | ||
| prepare_common_inputs, | ||
| require_cutoffs_for_family, | ||
| ) | ||
| from koopmans.aiida.workflows.blocks import ( | ||
| create_automatic_blocks, | ||
| create_explicit_blocks, | ||
|
|
@@ -310,9 +314,18 @@ def _build_wannierize_blocks_workgraph( | |
| scf_parameters.get("SYSTEM", {}).pop("nbnd", None) | ||
| nscf_parameters = copy.deepcopy(parameters) | ||
| nscf_parameters.setdefault("SYSTEM", {})["nbnd"] = nbnd | ||
| # This route assembles its own scf/nscf overrides instead of calling | ||
| # ``prepare_common_inputs``, so the cutoff check is its own too. | ||
|
Comment on lines
+317
to
+318
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, why?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same root cause, different reason for the split — and this one is a branch rather than a helper.
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 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 |
||
| require_cutoffs_for_family(pseudo_family, parameters) | ||
| wannier_overrides: WannierizeOverrides = { | ||
| "scf": {"pseudo_family": pseudo_family, "pw": {"parameters": scf_parameters}}, | ||
| "nscf": {"pseudo_family": pseudo_family, "pw": {"parameters": nscf_parameters}}, | ||
| "scf": { | ||
| "pseudo_family": pseudo_family, | ||
| "pw": {"parameters": scf_parameters}, | ||
| }, | ||
| "nscf": { | ||
| "pseudo_family": pseudo_family, | ||
| "pw": {"parameters": nscf_parameters}, | ||
| }, | ||
| } | ||
|
|
||
| # User wannier90 keywords (disentanglement windows, iteration counts, ...) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't it call prepare_common_inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
prepare_common_inputsbuilds a shape this route does not use, and resolves things it has already resolved.It returns
(structure, pseudo_family, overrides)whereoverridesis 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_inputsis a helper, called withstructureandnbndalready in hand from its caller, and what it assembles is aWannierizeOverridesfeeding a per-block fan-out. Callingprepare_common_inputswould 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
prepare_common_inputsdft,eps,dfptwannierize(whole-manifold)wannierize(block-by-block)dscf,trajectorySo 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.