Add injectable potentials to allow per-element PAW path and potType override#69
Add injectable potentials to allow per-element PAW path and potType override#69pmrv merged 4 commits intocalculatorfrom
Conversation
|
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 5 minor |
🟢 Metrics 10 complexity · 0 duplication
Metric Results Complexity 10 Duplication 0
TIP This summary will be updated as you push new changes. Give us feedback
pmrv
force-pushed
the
injectable-potentials
branch
from
2f7410f to
8248965
Compare
…verride
Adds an optional `potentials: dict[str, {potential, potType}]` parameter
to `get_paw_from_structure`, `get_paw_from_chemical_symbols`,
`set_base_parameters`, and `SphinxDft.__init__`. Elements absent from the
dict fall back to the existing `get_potential_path` / AtomPAW default.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pmrv
force-pushed
the
injectable-potentials
branch
from
8248965 to
fb23aa6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## calculator #69 +/- ##
==============================================
+ Coverage 85.90% 86.19% +0.28%
==============================================
Files 9 9
Lines 837 840 +3
==============================================
+ Hits 719 724 +5
+ Misses 118 116 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds an optional per-element potentials override that lets callers inject custom PAW potential paths and potType values, while preserving the existing default behavior (JTH GGA PBE under $CONDA_PREFIX + AtomPAW) for elements not overridden.
Changes:
- Extend PAW construction helpers to accept optional
potentialsoverrides (get_paw_from_structure,get_paw_from_chemical_symbols). - Thread
potentialsthrough the main input-builder (set_base_parameters) and the ASE calculator (SphinxDft). - Add unit tests validating injected vs fallback potential behavior and forwarding.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
sphinx_parser/potential.py |
Adds potentials support when building pawPot.species. |
sphinx_parser/jobs.py |
Forwards potentials into PAW setup from base input parameters. |
sphinx_parser/calculator.py |
Stores potentials on the calculator and forwards into input generation; adds/updates docstring. |
tests/unit/test_potential.py |
Adds unit tests for injected potentials + fallback behavior. |
tests/unit/test_jobs.py |
Adds test ensuring potentials is forwarded through set_base_parameters. |
CLAUDE.md |
Adds repository overview/developer notes. |
Comments suppressed due to low confidence (1)
sphinx_parser/jobs.py:18
set_base_parametersnow accepts apotentialsargument but the docstring’s Args section doesn’t mention it. Please document whatpotentialsis expected to contain (per-element override schema) and how elements not present fall back toget_potential_path/ defaultAtomPAW.
ekt: float = 0.2,
k_point_coords: list = [0.5, 0.5, 0.5],
potentials=None,
):
"""
Set the base parameters for the sphinx input file
Args:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| potentials (dict[str, dict] | None): Optional per-element PAW potential | ||
| overrides. Each key is an element symbol; each value is a dict with: | ||
| - ``potential`` (Path | str): path to the potential file. | ||
| - ``potType`` (str): potential type, e.g. ``"AtomPAW"`` or | ||
| ``"VASP"``. | ||
| Elements absent from this dict fall back to the JTH-GGA-PBE | ||
| potentials found under ``$CONDA_PREFIX``. | ||
| *args, **kwargs: Forwarded to :class:`ase.calculators.calculator.FileIOCalculator`. | ||
|
|
||
| Example:: | ||
|
|
||
| calc = SphinxDft(directory="./run") | ||
|
|
||
| calc = SphinxDft( | ||
| directory="./run", | ||
| potentials={ | ||
| "Fe": {"potential": Path("/pots/Fe_GGA.atomicdata"), "potType": "AtomPAW"}, | ||
| "Al": {"potential": Path("/pots/Al_VASP.POTCAR"), "potType": "VASP"}, | ||
| }, |
There was a problem hiding this comment.
The new potType documentation/examples here use "VASP", but the rest of this PR’s usage/tests use "VaspPAW". If potType is case-sensitive, this docstring could mislead users—please clarify the valid potType values and align the examples with what the library actually expects.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adds an optional
potentials: dict[str, {potential, potType}]parameter toget_paw_from_structure,get_paw_from_chemical_symbols,set_base_parameters, andSphinxDft.__init__. Elements absent from the dict fall back to the existingget_potential_path/ AtomPAW default.