Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 59 additions & 10 deletions src/koopmans/aiida/conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

from __future__ import annotations

import math
import warnings
from pathlib import Path
from typing import TYPE_CHECKING, Any

Expand Down Expand Up @@ -39,6 +41,10 @@ def _convert_paths_to_strings(obj: Any) -> Any:
# Quantum ESPRESSO's own value, so that converted quantities match QE output
BOHR_TO_ANGSTROM: float = CONSTANTS.bohr_to_ang

# ``ecutrho / ecutwfc`` for a norm-conserving pseudopotential, which is what
# kcp.x and kcw.x accept — and so the only kind koopmans runs.
NORM_CONSERVING_DUAL: float = 4.0


def code_parallelization(
config: CodeParallelization | None,
Expand Down Expand Up @@ -575,6 +581,54 @@ def kpoints_input_to_kpoints_path(
return kpts


def _resolve_pw_cutoffs(system: dict[str, Any], kcp_ecutrho: float) -> None:
"""Complete the ``SYSTEM`` cutoff pair in place, from ``ecutwfc``.

An unstated ``ecutrho`` becomes ``kcp_ecutrho``, or
``NORM_CONSERVING_DUAL * ecutwfc`` when that is unset too. Whatever its
source, an ``ecutrho`` that is not :data:`NORM_CONSERVING_DUAL` times
``ecutwfc`` takes effect with a warning naming the key it came from. With
neither cutoff stated the pair is left empty, for the pseudopotential
family to recommend.

Raises:
ValueError: If ``ecutrho`` is stated and ``ecutwfc`` is not.
"""
ecutwfc = system.get("ecutwfc")
ecutrho = system.get("ecutrho")
source = "calculator_parameters.pw.system.ecutrho"

if ecutwfc is None:
if ecutrho is not None:
raise ValueError(
"`calculator_parameters.pw.system.ecutrho` is set without a wavefunction "
"cutoff, which would pair it with whatever the pseudopotential family "
"recommends. Set `calculator_parameters.ecutwfc`; `ecutrho` follows at "
f"{NORM_CONSERVING_DUAL:g} times it on its own."
)
return

if ecutrho is None:
if kcp_ecutrho:
# An explicit ``kcp.system.ecutrho`` keeps the pw.x runs on the grid
# of the CP supercell run the dft_init consistency checks compare
# against.
ecutrho = kcp_ecutrho
source = "calculator_parameters.kcp.system.ecutrho"
else:
ecutrho = NORM_CONSERVING_DUAL * ecutwfc
system["ecutrho"] = ecutrho

if not math.isclose(ecutrho, NORM_CONSERVING_DUAL * ecutwfc):
warnings.warn(
f"`{source}` = {ecutrho:g} Ry: ecutrho should be {NORM_CONSERVING_DUAL:g} x "
f"ecutwfc = {NORM_CONSERVING_DUAL * ecutwfc:g} Ry for norm-conserving "
f"pseudopotentials. Drop `{source}` to take that.",
UserWarning,
stacklevel=3,
)


def input_to_pw_parameters(koopmans_input: KoopmansInput) -> dict[str, dict[str, Any]]:
"""Convert KoopmansInput to a PW input-parameter namelist dict.

Expand All @@ -596,16 +650,6 @@ def input_to_pw_parameters(koopmans_input: KoopmansInput) -> dict[str, dict[str,
# Add ecutwfc if specified
if calc_params.ecutwfc is not None:
parameters["SYSTEM"]["ecutwfc"] = calc_params.ecutwfc
# Pin ecutrho alongside it (kcp.x convention: 4x for norm-conserving,
# same fallback as ``_extract_kcp_scalar_inputs``). Without this the
# pseudo family's placeholder ecutrho wins in protocol-built PW runs,
# putting them on a different grid from the CP supercell run that the
# dft_init consistency checks compare against. An explicit
# ``pw.system.ecutrho`` still overrides via the update below.
kcp_system = calc_params.kcp.system
parameters["SYSTEM"]["ecutrho"] = (
kcp_system.ecutrho if kcp_system.ecutrho else 4.0 * calc_params.ecutwfc
)

# Add nbnd if specified
if calc_params.nbnd is not None:
Expand All @@ -624,6 +668,11 @@ def input_to_pw_parameters(koopmans_input: KoopmansInput) -> dict[str, dict[str,
parameters["ELECTRONS"].update(
pw_params.electrons.model_dump(exclude_none=True, exclude_defaults=True)
)

# After the merge, so the pair is resolved from the cutoffs pw.x will run
# rather than from the top-level shorthand a ``pw.system`` block may replace.
_resolve_pw_cutoffs(parameters["SYSTEM"], calc_params.kcp.system.ecutrho)

# Ensure all Path objects are converted to strings for JSON serialization
parameters = _convert_paths_to_strings(parameters)

Expand Down
23 changes: 20 additions & 3 deletions src/koopmans/aiida/setup/pseudos.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,22 @@ def ensure_pseudo_family_installed(pseudo_family: str) -> None:
logger.info("Successfully installed pseudo family '%s'", pseudo_family)


def pseudo_family_has_cutoffs(pseudo_family: str) -> bool:
"""Report whether an installed family publishes recommended cutoffs.

True only if the family defines at least one cutoff stringency; without one
``get_recommended_cutoffs`` has nothing to return.

Raises:
NotExistent: If the family is not installed.
"""
from aiida_pseudo.groups.family import PseudoPotentialFamily

family = PseudoPotentialFamily.collection.get(label=pseudo_family)
stringencies = getattr(family, "get_cutoff_stringencies", None)
return stringencies is not None and bool(stringencies())


def install_pseudo_family(pseudo_family: str) -> None:
"""Install a pseudopotential family. Parse the label and dispatch."""
parts = pseudo_family.split("/")
Expand Down Expand Up @@ -179,9 +195,10 @@ def _install_sssp_family(label: str, parts: list[str]) -> None:
# SG15 ONCV is published as a single frozen tarball on quantum-simulation.org. It
# bundles every version x relativistic variant in one flat archive; the label's
# version/relativistic parts select which subset of UPFs to install. There is no
# upstream ``aiida-pseudo`` installer for SG15, so we install as a plain
# ``CutoffsPseudoPotentialFamily`` so recommended cutoffs can be attached later
# via ``family.set_cutoffs`` without a reinstall.
# upstream ``aiida-pseudo`` installer for SG15, so we install a
# ``CutoffsPseudoPotentialFamily`` directly. SG15 publishes no recommended
# cutoffs, so the family carries none and the input file must set ``ecutwfc``
# and ``ecutrho`` itself.
_SG15_ARCHIVE_URL = (
"http://www.quantum-simulation.org/potentials/sg15_oncv/sg15_oncv_upf_2020-02-06.tar.gz"
)
Expand Down
40 changes: 39 additions & 1 deletion src/koopmans/aiida/workflows/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,40 @@ def load_codes_for_task(workflow: WorkflowConfig) -> Codes:
return codes


def pw_pseudo_overrides(
pseudo_family: str,
structure: orm.StructureData,
parameters: dict[str, Any],
) -> dict[str, Any]:
"""Return the ``pw`` override entries that pin a cutoff-less family's pseudos.

Empty for a family that publishes recommended cutoffs. Otherwise the
family's pseudos, which aiida-quantumespresso's protocol builder accepts
only when ``parameters['SYSTEM']`` carries both cutoffs — a pair
:func:`~koopmans.aiida.conversion.input_to_pw_parameters` completes from
``ecutwfc`` alone.

Raises:
ValueError: If the family publishes no recommended cutoffs and the
input states none either.
"""
from koopmans.aiida.conversion import get_pseudos_from_family
from koopmans.aiida.setup.pseudos import pseudo_family_has_cutoffs

if pseudo_family_has_cutoffs(pseudo_family):
return {}

if "ecutwfc" not in parameters.get("SYSTEM", {}):
raise ValueError(
f"The pseudopotential family `{pseudo_family}` publishes no recommended "
"cutoffs, so they must come from the input file: set "
"`calculator_parameters.ecutwfc`. `ecutrho` follows at four times it "
"unless `calculator_parameters.pw.system.ecutrho` states otherwise."
)

return {"pseudos": get_pseudos_from_family(pseudo_family, structure)}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def prepare_common_inputs(
koopmans_input: KoopmansInput,
override_keys: list[str],
Expand All @@ -123,7 +157,9 @@ def prepare_common_inputs(

Converts the koopmans input into a structure, ensures the pseudo family is
installed, and builds an overrides dict with a PW parameters entry for each
of the requested sub-workflow keys.
of the requested sub-workflow keys. A family publishing no recommended
cutoffs also has its pseudos pinned in that entry
(:func:`pw_pseudo_overrides`).

Args:
koopmans_input: The parsed koopmans input.
Expand All @@ -141,6 +177,8 @@ def prepare_common_inputs(
ensure_pseudo_family_installed(pseudo_family)

pw_overrides: dict[str, Any] = {"parameters": parameters}
pw_overrides.update(pw_pseudo_overrides(pseudo_family, structure, parameters))

# The pw entry carries the pw.x parallelization directive: -npool rides
# settings.cmdline; ntasks rides metadata.options.resources — both survive
# get_builder_from_protocol's override merge (verified by eager build).
Expand Down
7 changes: 4 additions & 3 deletions src/koopmans/aiida/workflows/dscf.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
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 load_code, reject_kpoint_overrides
from koopmans.aiida.workflows import load_code, pw_pseudo_overrides, reject_kpoint_overrides
from koopmans.aiida.workflows.blocks import (
create_explicit_blocks,
validate_blocks_cover_all_occ_bands,
Expand Down Expand Up @@ -210,9 +210,10 @@ def dscf_wannier_init_inputs(
validate_blocks_separate_occ_and_emp(blocks, nocc)
validate_blocks_cover_all_occ_bands(blocks, nocc)

pinned = pw_pseudo_overrides(pseudo_family, structure, parameters)
wannier_overrides: WannierizeOverrides = {
"scf": {"pseudo_family": pseudo_family, "pw": {"parameters": parameters}},
"nscf": {"pseudo_family": pseudo_family, "pw": {"parameters": parameters}},
"scf": {"pseudo_family": pseudo_family, "pw": {"parameters": parameters, **pinned}},
"nscf": {"pseudo_family": pseudo_family, "pw": {"parameters": parameters, **pinned}},
}

# User wannier90 keywords (disentanglement windows, iteration counts, ...)
Expand Down
13 changes: 10 additions & 3 deletions src/koopmans/aiida/workflows/wannierize.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
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, pw_pseudo_overrides
from koopmans.aiida.workflows.blocks import (
create_automatic_blocks,
create_explicit_blocks,
Expand Down Expand Up @@ -310,9 +310,16 @@ def _build_wannierize_blocks_workgraph(
scf_parameters.get("SYSTEM", {}).pop("nbnd", None)
nscf_parameters = copy.deepcopy(parameters)
nscf_parameters.setdefault("SYSTEM", {})["nbnd"] = nbnd
pinned = pw_pseudo_overrides(pseudo_family, structure, 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, **pinned},
},
"nscf": {
"pseudo_family": pseudo_family,
"pw": {"parameters": nscf_parameters, **pinned},
},
}

# User wannier90 keywords (disentanglement windows, iteration counts, ...)
Expand Down
35 changes: 34 additions & 1 deletion src/koopmans/input_file/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ def check_up_down_exclusivity(self) -> Wannier90InputParametersWithUpDown:
class CalculatorParametersInput(BaseModel):
"""Calculator-specific input parameters."""

ecutwfc: float | None = None
ecutwfc: float | None = Field(default=None, gt=0.0)
nbnd: int | None = None
tot_magnetization: float | None = None
pw: PWInputParameters = Field(default_factory=lambda: PWInputParameters())
Expand All @@ -336,6 +336,38 @@ class CalculatorParametersInput(BaseModel):
)
kcp: KCPInputParameters = Field(default_factory=lambda: KCPInputParameters())

@model_validator(mode="after")
def check_cutoffs_agree(self) -> CalculatorParametersInput:
"""Validate that the stated wavefunction cutoffs agree, and the density ones.

pw.x and kcp.x read their cutoffs from different keys, so two stated
values that differ put the two codes on different grids. A
``kcp.system`` cutoff of zero is unset rather than a stated zero.

Raises:
ValueError: If two keys state the same cutoff at different values.
"""
wavefunction = {
"calculator_parameters.ecutwfc": self.ecutwfc,
"calculator_parameters.pw.system.ecutwfc": self.pw.system.ecutwfc,
"calculator_parameters.kcp.system.ecutwfc": self.kcp.system.ecutwfc or None,
}
density = {
"calculator_parameters.pw.system.ecutrho": self.pw.system.ecutrho,
"calculator_parameters.kcp.system.ecutrho": self.kcp.system.ecutrho or None,
}

for cutoff, keys in (("wavefunction", wavefunction), ("density", density)):
stated = {key: value for key, value in keys.items() if value is not None}
if len(set(stated.values())) > 1:
listed = [f"`{key}` = {value:g} Ry" for key, value in stated.items()]
raise ValueError(
f"{', '.join(listed[:-1])} and {listed[-1]} disagree. pw.x and kcp.x "
f"must run on the same grid, so state one {cutoff} cutoff: give these "
"keys the same value, or drop all but one of them."
)
return self


class KoopmansInput(BaseModel):
"""Input schema for ``koopmans`` input files."""
Expand Down Expand Up @@ -440,6 +472,7 @@ def _resolve(path: str) -> str:
"type": 'is the wrong type (should be "{expected_type}", not "{given_type}")',
"extra_forbidden": "is not a valid keyword.",
"missing": "was not provided.",
"greater_than": "must be greater than {gt}.",
}


Expand Down
5 changes: 3 additions & 2 deletions src/koopmans/input_file/pw.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,16 @@ class SystemNamelist(_SystemNamelist):

``ibrav``, ``nat`` and ``ntyp`` are derived from the input structure, not provided
by the user. ``ecutwfc`` is optional at parse time; the dispatcher raises if it is
still unset when the workgraph is built.
still unset when the workgraph is built. Both cutoffs must be greater than zero.
"""

# Excluded fields (see ``ControlNamelist`` above for the ClassVar rationale).
ibrav: ClassVar[int | None] = None # type: ignore[misc, assignment, unused-ignore]
nat: ClassVar[int | None] = None # type: ignore[misc, assignment, unused-ignore]
ntyp: ClassVar[int | None] = None # type: ignore[misc, assignment, unused-ignore]
# Optional at parse time; the dispatcher raises if still unset at build time.
ecutwfc: float | None = None # type: ignore[assignment]
ecutwfc: float | None = Field(default=None, gt=0.0) # type: ignore[assignment]
ecutrho: float | None = Field(default=None, gt=0.0)


class PWInputParameters(BaseModel):
Expand Down
2 changes: 2 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
compiled_binaries,
fake_pseudodojo_lda_family,
fake_sg15_cutoffs_family,
fake_sg15_family_without_cutoffs,
fake_sg15_fr_cutoffs_family,
fake_sg15_pseudo_family,
fake_user_built_family,
installed_decompose_code,
installed_fold_codes,
installed_kcp_code,
Expand Down
Loading