-
Notifications
You must be signed in to change notification settings - Fork 198
docs: define PWmat first-principles arguments #1961
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import textwrap | ||
| from typing import Union | ||
| from typing import Optional, Union | ||
|
|
||
| from dargs import Argument, Variant | ||
| from dargs.dargs import ArgumentValueError | ||
|
|
||
| from dpgen.arginfo import general_mdata_arginfo | ||
|
|
||
|
|
@@ -987,6 +988,155 @@ def fp_style_custom_args() -> list[Argument]: | |
| ] | ||
|
|
||
|
|
||
| def fp_style_pwmat_args() -> list[Argument]: | ||
| """Return first-principles arguments for PWmat labeling.""" | ||
| required_generated_keys = { | ||
| "node1", | ||
| "node2", | ||
| "in.atom", | ||
| "ecut", | ||
| "e_error", | ||
| "rho_error", | ||
| "kspacing", | ||
| "flag_symm", | ||
| } | ||
|
|
||
| def has_required_generated_keys(params): | ||
| return required_generated_keys.issubset(params) | ||
|
|
||
| generated_args = [ | ||
| Argument("node1", int, optional=False, doc="First PWmat node-grid size."), | ||
| Argument("node2", int, optional=False, doc="Second PWmat node-grid size."), | ||
| Argument( | ||
| "in.atom", | ||
| str, | ||
| optional=False, | ||
| doc="Atom-configuration filename written to the PWmat input.", | ||
| ), | ||
| Argument( | ||
| "ecut", | ||
| [int, float], | ||
| optional=False, | ||
| doc="Plane-wave energy cutoff.", | ||
| ), | ||
| Argument( | ||
| "e_error", | ||
| [int, float], | ||
| optional=False, | ||
| doc="Electronic-energy convergence threshold.", | ||
| ), | ||
| Argument( | ||
| "rho_error", | ||
| [int, float], | ||
| optional=False, | ||
| doc="Charge-density convergence threshold.", | ||
| ), | ||
| Argument( | ||
| "kspacing", | ||
| [int, float], | ||
| optional=False, | ||
| doc="Reciprocal-space spacing used to generate MP_N123.", | ||
| ), | ||
| Argument( | ||
| "flag_symm", | ||
| [int, str], | ||
| optional=True, | ||
| default="NONE", | ||
| doc="PWmat symmetry flag: 0, 1, 2, 3, or 'NONE'.", | ||
|
Comment on lines
+1041
to
+1045
Contributor
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- applicable repository knowledge ---'
find /tmp/coderabbit-repo-knowledge/deepmodeling-dpgen-4dbd1441 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- scoped convention headers ---'
for f in /tmp/coderabbit-repo-knowledge/deepmodeling-dpgen-4dbd1441/*/*.md; do
[ -f "$f" ] && { printf '\n### %s\n' "$f"; head -80 "$f"; }
done
printf '%s\n' '--- target file outline and hunk ---'
ast-grep outline dpgen/generator/arginfo.py | sed -n '1,180p'
sed -n '980,1070p' dpgen/generator/arginfo.py
printf '%s\n' '--- dargs references and nearby tests ---'
rg -n --glob '!*.lock' 'dargs|flag_symm|fp_style_pwmat_args|normalize_value|check_value' pyproject.toml setup.py setup.cfg requirements* dpgen tests 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- candidate test files ---'
git ls-files | rg '(^|/)(test|tests)(/|_)|arginfo'Repository: deepmodeling/dpgen Length of output: 41608 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- PWmat schema registration ---'
sed -n '1110,1165p' dpgen/generator/arginfo.py
printf '%s\n' '--- PWmat schema tests ---'
cat -n tests/test_pwmat_arginfo.py
printf '%s\n' '--- PWmat consumer ---'
sed -n '145,210p' dpgen/generator/lib/pwmat.py
printf '%s\n' '--- generator call path ---'
sed -n '3135,3195p' dpgen/generator/run.py
printf '%s\n' '--- dargs declaration and local availability ---'
sed -n '1,70p' pyproject.toml
python3 - <<'PY'
import importlib.util
spec = importlib.util.find_spec("dargs")
print("dargs_spec", spec)
if spec:
import dargs
print("dargs_version", getattr(dargs, "__version__", "<no __version__>"))
print("dargs_file", getattr(dargs, "__file__", "<no file>"))
PY
printf '%s\n' '--- dargs source files in repository, if any ---'
git ls-files | rg '(^|/)(dargs|.*dargs.*)(/|$)' || trueRepository: deepmodeling/dpgen Length of output: 12608 🌐 Web query:
💡 Result: The dargs library is a tool for validating and normalizing dictionary-based input arguments in Python programs [1][2]. In version 0.4.0, the central class is dargs.Argument, which defines the structure of expected arguments [3][4]. Key components and methods: Argument Definition - sub_fields: An optional iterable of Argument objects. When provided, the argument's dtype is treated as a dictionary, where items must correspond to the arguments defined in sub_fields [4]. - extra_check: A callable that takes a value and returns a boolean; it provides a way to perform custom, additional validation on the argument value [3][4]. Validation and Normalization Methods - check(argdict, strict=False, allow_ref=False): Validates if a dictionary conforms to the Argument structure [5][4]. - strict: When set to True, only keys explicitly defined in the Argument object are allowed [5][4]. - allow_ref: When set to True, enables the resolution of $ref keys pointing to external JSON or YAML files [6][4]. - check_value(value, strict=False, allow_ref=False): Similar to check, but designed to validate a value without requiring the leading key [3][1]. - normalize(argdict, inplace=False,...): Modifies an argument dictionary by handling aliases and applying default values defined in the Argument structure [3][1]. - normalize_value(value, inplace=False,...): The equivalent of normalize for values, ignoring the leading key [3][1]. Citations:
🌐 Web query:
💡 Result: In the context of the dargs library (often used for processing arguments in deep modeling projects) [1][2], the methods mentioned are core components of the Argument class used to manage and validate input dictionaries. - check_value: This method takes a dictionary and validates whether its structure and types conform to the definitions provided in the Argument class [1]. It differs from the base check method by ignoring the leading key when comparing the input to the defined structure [1]. - normalize_value: This method processes a dictionary to handle aliases and inject default values [1]. Similar to check_value, it operates by ignoring the leading key relative to the standard normalize method [1]. - extra_check: While the primary methods are check/check_value and normalize/normalize_value, extra_check (often implemented as a callback or extension in such frameworks) is typically used to perform custom, user-defined validation logic on the argument values that goes beyond simple type checking [1]. The version v0.4.0 is part of the development history of the dargs library, which is maintained by DeepModeling [1][3]. These methods allow for flexible configuration management where complex argument structures can be defined, validated, and normalized dynamically [1]. Citations:
🏁 Script executed: #!/bin/bash
set -e
base='https://raw.githubusercontent.com/deepmodeling/dargs/v0.4.0'
for path in dargs/dargs.py dargs/__init__.py; do
echo "--- $path ---"
curl -fsSL "$base/$path" | rg -n -C 8 'class Argument|extra_check|def check_value|def normalize_value|check_value\(' || true
done
echo '--- tag metadata ---'
curl -fsSL 'https://api.github.com/repos/deepmodeling/dargs/git/ref/tags/v0.4.0' |
python3 -c 'import json,sys; d=json.load(sys.stdin); print(json.dumps(d, indent=2)[:4000])'Repository: deepmodeling/dpgen Length of output: 8085 🏁 Script executed: #!/bin/bash
set -e
curl -fsSL 'https://raw.githubusercontent.com/deepmodeling/dargs/v0.4.0/dargs/dargs.py' |
sed -n '350,455p;490,565p'Repository: deepmodeling/dpgen Length of output: 6602 Constrain
🤖 Prompt for AI Agents |
||
| ), | ||
| Argument( | ||
| "icmix", | ||
| [int, float], | ||
| optional=True, | ||
| doc="SCF mixing parameter used to build scf_iter0_2.", | ||
| ), | ||
| Argument( | ||
| "smearing", | ||
| int, | ||
| optional=True, | ||
| doc="PWmat smearing method written to SCF iteration settings.", | ||
| ), | ||
| Argument( | ||
| "sigma", | ||
| [int, float], | ||
| optional=True, | ||
| doc="Smearing width written to SCF iteration settings.", | ||
| ), | ||
| Argument( | ||
| "user_pwmat_params", | ||
| dict, | ||
| optional=True, | ||
| doc="Arbitrary PWmat keys overriding the generated input dictionary.", | ||
| ), | ||
| ] | ||
|
|
||
| return [ | ||
| Argument( | ||
| "fp_pp_path", | ||
| str, | ||
| optional=False, | ||
| doc="Directory containing PWmat pseudopotential files.", | ||
| ), | ||
| Argument( | ||
| "fp_pp_files", | ||
| list[str], | ||
| optional=False, | ||
| doc="Pseudopotential filenames ordered consistently with type_map.", | ||
| ), | ||
| Argument( | ||
| "fp_incar", | ||
| str, | ||
| optional=True, | ||
| doc="Existing etot.input template; this takes highest priority.", | ||
| ), | ||
| Argument( | ||
| "user_fp_params", | ||
| dict, | ||
| optional=True, | ||
| extra_check=has_required_generated_keys, | ||
| extra_check_errmsg=( | ||
| "user_fp_params must define node1, node2, in.atom, ecut, " | ||
| "e_error, rho_error, kspacing, and flag_symm" | ||
| ), | ||
| doc=( | ||
| "Compatibility input mapping. The current generator consumes " | ||
| "node1, node2, in.atom, ecut, e_error, rho_error, kspacing, and " | ||
| "flag_symm, regenerates etot.input, and ignores other keys." | ||
| ), | ||
| ), | ||
| Argument( | ||
| "fp_params", | ||
| dict, | ||
| optional=True, | ||
|
coderabbitai[bot] marked this conversation as resolved.
njzjz-bot marked this conversation as resolved.
|
||
| sub_fields=generated_args, | ||
| doc=( | ||
| "Parameters used by make_pwmat_input_user_dict when neither " | ||
| "fp_incar nor user_fp_params is supplied." | ||
| ), | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| class _FpStyleVariant(Variant): | ||
| """Validate cross-field requirements for first-principles backends.""" | ||
|
|
||
| def get_choice(self, argdict: dict, path: Optional[list[str]] = None) -> Argument: | ||
| """Return the selected backend after validating PWmat input sources. | ||
|
|
||
| Dargs flattens variant fields into their parent mapping, so a regular | ||
| field-level extra check cannot express this at-least-one constraint. | ||
| """ | ||
| choice = super().get_choice(argdict, path) | ||
| input_sources = {"fp_incar", "user_fp_params", "fp_params"} | ||
| if choice.name == "pwmat" and input_sources.isdisjoint(argdict): | ||
| raise ArgumentValueError( | ||
| path, | ||
| "PWmat requires at least one input source: fp_incar, " | ||
| "user_fp_params, or fp_params.", | ||
| ) | ||
| return choice | ||
|
|
||
|
|
||
| def fp_style_variant_type_args() -> Variant: | ||
| doc_fp_style = "Software for First Principles." | ||
| doc_amber_diff = ( | ||
|
|
@@ -1001,8 +1151,12 @@ def fp_style_variant_type_args() -> Variant: | |
| "The command argument in the machine file should be the script to run custom FP codes. " | ||
| "The extra forward and backward files can be defined in the machine file." | ||
| ) | ||
| doc_pwmat = ( | ||
| "PWmat density-functional labeling. The machine command should invoke " | ||
| "the site-specific PWmat executable." | ||
| ) | ||
|
|
||
| return Variant( | ||
| return _FpStyleVariant( | ||
| "fp_style", | ||
| [ | ||
| Argument("vasp", dict, fp_style_vasp_args()), | ||
|
|
@@ -1013,7 +1167,7 @@ def fp_style_variant_type_args() -> Variant: | |
| Argument( | ||
| "amber/diff", dict, fp_style_amber_diff_args(), doc=doc_amber_diff | ||
| ), | ||
| Argument("pwmat", dict, [], doc="TODO: add doc"), | ||
| Argument("pwmat", dict, fp_style_pwmat_args(), doc=doc_pwmat), | ||
|
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- repository conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/deepmodeling-dpgen-4dbd1441 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- changed registration and nearby definitions ---'
sed -n '960,1025p' dpgen/generator/arginfo.py
sed -n '1125,1160p' dpgen/generator/arginfo.py
printf '%s\n' '--- simplify registration and nearby definitions ---'
rg -n -C 8 'pwmat|fp_pp_path|fp_pp_files' dpgen/simplify/arginfo.py
printf '%s\n' '--- dargs validation usage ---'
rg -n -C 4 'Argument\(|dargs|check_value|extra_check' dpgen/generator/arginfo.py dpgen/simplify/arginfo.py | head -240Repository: deepmodeling/dpgen Length of output: 20878 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- PWmat schema ---'
sed -n '1000,1135p' dpgen/generator/arginfo.py
printf '%s\n' '--- simplify imports and validation path ---'
sed -n '1,155p' dpgen/simplify/arginfo.py
sed -n '185,245p' dpgen/simplify/arginfo.py
rg -n -C 6 'simplify_jdata_arginfo|check_value|normalize|dargs' dpgen/simplify dpgen | head -240
printf '%s\n' '--- relevant repository conventions ---'
cat /tmp/coderabbit-repo-knowledge/deepmodeling-dpgen-4dbd1441/conventions/dpgen.md
cat /tmp/coderabbit-repo-knowledge/deepmodeling-dpgen-4dbd1441/conventions/pyproject-toml.mdRepository: deepmodeling/dpgen Length of output: 25601 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- dargs version and strict empty-dict behavior ---'
python3 - <<'PY'
import inspect
import dargs
from dargs import Argument
print("dargs_version:", getattr(dargs, "__version__", "unknown"))
print("Argument.check_value source:")
print(inspect.getsource(Argument.check_value))
PY
printf '%s\n' '--- focused tests and PWmat consumers ---'
rg -n -C 8 'pwmat|fp_pp_path|fp_pp_files|simplify_jdata_arginfo|normalize\(' tests dpgen/simplify dpgen/generator | head -300Repository: deepmodeling/dpgen Length of output: 327 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- dargs dependency declaration ---'
rg -n -C 3 'dargs' pyproject.toml setup.py setup.cfg requirements* environment*.yml 2>/dev/null || true
printf '%s\n' '--- focused tests and PWmat consumers ---'
rg -n -C 8 'pwmat|fp_pp_path|fp_pp_files|simplify_jdata_arginfo|normalize\(' tests dpgen/simplify dpgen/generator 2>/dev/null | head -300Repository: deepmodeling/dpgen Length of output: 22183 🌐 Web query:
💡 Result: In the dargs library (version 0.4.0), the check_value method is used to validate a value against an Argument structure without requiring the leading key [1][2]. Regarding the behavior of strict validation, sub_fields, and unknown keys: 1. Strict Validation: When the strict parameter is set to True, the validation process only permits keys that are explicitly defined in the Argument object [2][3]. If an unknown key is encountered while strict=True, an error will be raised [2]. 2. Sub_fields: The check_value method recursively validates nested dictionaries (sub-arguments) based on the sub_fields and sub_variants defined within the Argument object [2]. 3. Empty/Unknown Key Behavior: The library uses strict mode to control whether unknown keys are allowed. By default, or when strict is set to False, keys not explicitly defined in the argument structure may be accepted, depending on the specific implementation context. When strict=True, any key not defined in the corresponding Argument's sub_fields or sub_variants is considered unknown and triggers a validation failure [2][3]. There is no specific behavior identified for "empty" keys other than the standard validation of structure and types for the provided dictionary [1][4]. Documentation notes that the library examines the argument's type as well as the keys and types of its sub-arguments [1][4]. Citations:
Keep the simplify PWmat schema compatible.
Add the equivalent PWmat schema to 🤖 Prompt for AI Agents |
||
| Argument("pwscf", dict, fp_style_pwscf_args()), | ||
| Argument("cpx", dict, fp_style_cpx_args()), | ||
| Argument("custom", dict, fp_style_custom_args(), doc=doc_custom), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| """Validate PWmat first-principles parameter documentation.""" | ||
|
|
||
| import unittest | ||
|
|
||
| from dargs import Argument | ||
| from dargs.dargs import ArgumentValueError | ||
|
|
||
| from dpgen.generator.arginfo import fp_style_variant_type_args | ||
|
|
||
|
|
||
| class TestPWmatArginfo(unittest.TestCase): | ||
| def setUp(self): | ||
| self.arginfo = Argument("fp", dict, sub_variants=[fp_style_variant_type_args()]) | ||
| self.pseudopotentials = { | ||
| "fp_style": "pwmat", | ||
| "fp_pp_path": ".", | ||
| "fp_pp_files": ["C.UPF", "H.UPF"], | ||
| } | ||
|
|
||
| def check(self, data): | ||
| normalized = self.arginfo.normalize_value(data) | ||
| self.arginfo.check_value(normalized, strict=True) | ||
|
|
||
| def test_existing_input_file(self): | ||
| self.check({**self.pseudopotentials, "fp_incar": "etot.input"}) | ||
|
|
||
| def test_requires_an_input_source(self): | ||
| """A valid PWmat configuration must select one supported input path.""" | ||
| with self.assertRaisesRegex(ArgumentValueError, "at least one input source"): | ||
| self.check(self.pseudopotentials) | ||
|
|
||
| def test_generated_fp_params(self): | ||
| self.check( | ||
| { | ||
| **self.pseudopotentials, | ||
| "fp_params": { | ||
| "node1": 4, | ||
| "node2": 1, | ||
| "in.atom": "atom.config", | ||
| "ecut": 50, | ||
| "e_error": 1e-4, | ||
| "rho_error": 1e-4, | ||
| "kspacing": 0.1, | ||
| "flag_symm": "NONE", | ||
| "icmix": 1.0, | ||
| "smearing": 2, | ||
| "sigma": 0.025, | ||
| "user_pwmat_params": {"job": "SCF", "out.wg": False}, | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| def test_generated_fp_params_default_symmetry(self): | ||
| """PWmat accepts generated inputs without an explicit symmetry override.""" | ||
| self.check( | ||
| { | ||
| **self.pseudopotentials, | ||
| "fp_params": { | ||
| "node1": 4, | ||
| "node2": 1, | ||
| "in.atom": "atom.config", | ||
| "ecut": 50, | ||
| "e_error": 1e-4, | ||
| "rho_error": 1e-4, | ||
| "kspacing": 0.1, | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| def test_compatibility_user_fp_params(self): | ||
| self.check( | ||
| { | ||
| **self.pseudopotentials, | ||
| "user_fp_params": { | ||
| "node1": 4, | ||
| "node2": 1, | ||
| "job": "SCF", | ||
| "in.atom": "atom.config", | ||
| "in.psp1": "C.UPF", | ||
| "in.psp2": "H.UPF", | ||
| "ecut": 50, | ||
| "flag_symm": 2, | ||
| "e_error": 1e-4, | ||
| "rho_error": 1e-4, | ||
| "scf_iter0_1": "6 4 3 0.0000 0.025 2", | ||
| "xcfunctional": "PBE", | ||
| "kspacing": 0.1, | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
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.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use NumPy-style docstrings for the new functions.
Add a
Returnssection tofp_style_pwmat_args(). AddParametersandReturnssections tohas_required_generated_keys().As per coding guidelines,
dpgen/**/*.pymust use NumPy-style docstrings for functions and classes.Also applies to: 1003-1003
🤖 Prompt for AI Agents
Source: Coding guidelines