Expose the scf/nscf recipe as get_scf_nscf_builders_from_protocol - #93
Open
elinscott wants to merge 5 commits into
Open
Expose the scf/nscf recipe as get_scf_nscf_builders_from_protocol#93elinscott wants to merge 5 commits into
elinscott wants to merge 5 commits into
Conversation
External callers that orchestrate their own wannierisation (e.g. one shared nscf reused by several per-block Wannier90 runs) previously had to copy the protocol's nscf invariants (nosym/noinv, diago_full_acc, the wannier90-ordered explicit k-list, the nbnd bookkeeping) by hand - and silently drifted when they changed. The scf/nscf section of get_builder_from_protocol now lives in a standalone classmethod that the main builder delegates to, so there is exactly one copy of the recipe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Note: tests expected to fail until #95 is merged |
get_builder_from_protocol delegated scf/nscf construction to get_scf_nscf_builders_from_protocol without forwarding the resolved pseudo_family, and the new method wrote pseudo_family=None into both sub-builder overrides unconditionally. Symptom: get_builder_from_protocol callers and any direct caller at the method's own defaults raised "ValueError: required pseudo family `None` is not installed" (seen across test_bands, test_open_grid, test_optimize, test_protocols). Forward pseudo_family in the delegation call and only set the override when it is not None so the standalone API falls back to the protocol default. Add a test exercising get_scf_nscf_builders_from_protocol directly without pseudo_family. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
get_scf_nscf_builders_from_protocol already pops kpoints_distance and assigns the explicit wannier90 k-list it is passed, so the main method re-popping and re-assigning the same object afterwards was dead work. The wannier builder supplies an explicit KpointsData, which the new method uses as-is, so the net nscf.kpoints is unchanged. Co-Authored-By: Claude Fable 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
1 participant
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
Wannier90WorkChain.get_builder_from_protocolencodes a number of settings into its scf and nscf builders:nosym/noinvso the nscf produces the full unshifted k-grid that wannier90 and pw2wannier90 requirediago_full_accfor well-converged empty stateskmesh.plordering so the k-points cannot drift between pw.x and wannier90nbnd = num_bands + len(exclude_bands)bookkeeping.External orchestrators that run their own scf & nscf — in my case, a workflow that wannierises several projection blocks off one shared nscf — currently have to copy these settings by hand, and would silently drift if/when the protocol changes
Change
The scf/nscf section of
get_builder_from_protocolmoves into a new public classmethod,Wannier90WorkChain.get_scf_nscf_builders_from_protocol(code, *, structure, nbnd=None, kpoints=None, protocol=None, overrides=None, ...), which returns the twoPwBaseWorkChainbuilders. The main builder now delegates to it, so the recipe exists in exactly one place and cannot diverge.Notes
get_builder_from_protocolcallers: the delegation reproduces the previous inputs.overridestakes the same{'scf': ..., 'nscf': ...}shape as the main builder.