Skip to content

Add GPAW competing-phase output parsing for chemical potentials - #161

Merged
kavanase merged 11 commits into
SMTG-Bham:IO_refactorfrom
sblisesivdin:feat/gpaw-competing-phase-parsing
Nov 1, 2026
Merged

kavanase merged 11 commits into
SMTG-Bham:IO_refactorfrom
sblisesivdin:feat/gpaw-competing-phase-parsing

Conversation

@sblisesivdin

@sblisesivdin sblisesivdin commented Oct 30, 2026 •

Copy link
Copy Markdown

This PR adds GPAW competing-phase output parsing for chemical-potential analysis directly from restart files:

phase_analyzer = CompetingPhasesAnalyzer(
"MgO",
entries="CompetingPhases_GPAW",
calculator="gpaw",
)

The MgO example includes the host and elemental references in this directory and uses the analyzer in its analysis script.

The parser reads structures, stored energies and metadata without loading defect electrostatic potentials or orbital data. Charged calculations and restarts without stored energy are rejected, preventing unintended SCF calculations. The stored-energy guard also applies to the general GPAW output reader.

File discovery follows GPAW filename priorities and excludes backups. Entries record actual k-point meshes, including density-based sampling. Compatibility checks warn about inconsistent energy-affecting settings while allowing different k-point and real-space meshes.

Validation with GPAW 26.7:

  • 13 phase/restart tests passed using real MgO, Mg and triplet O₂ calculations and shipped restart files, without mocked calculators.
  • Tests cover directory discovery, Mg-rich/O-rich limits, serialization, file priority, charge and missing-energy rejection, k-point meshes, and XC/PAW/mode mismatches.
  • One existing GPAW output-reader test and two existing VASP discovery tests passed.
  • Applicable pre-commit hooks and example lint/format checks passed.

Ionic convergence cannot be established from a restart alone. Band gaps and general GPAW competing-phase input generation remain outside this PR. The full test suite was not run.

Related to #156; this does not close the full tracking issue.

@SMTG-Bham SMTG-Bham deleted a comment from coderabbitai Bot Oct 31, 2026
@kavanase

Copy link
Copy Markdown
Member

Thank you for working on this @sblisesivdin!

Some initial comments on this PR:

  1. The documented call seems to fail on the example layout. CompetingPhasesAnalyzer("MgO", entries=["MgO_bulk", "CompetingPhases_GPAW"], calculator="gpaw") (in docs/GPAW_Support.rst:180, also in the PR description) only parses MgO_bulk. It drops CompetingPhases_GPAW/<phase>/ with no warning, then fails with ValueError: No elemental reference phase was parsed for host element(s): ['Mg', 'O'].
    • Origin: directories given in a list are not searched recursively (doped/io/utils.py:64 only looks one level down). This is generic and pre-existing, and affects VASP too. The docstring at chemical_potentials.py:3199 claims the search is recursive.
    • Fix (checked): add "MgO_Fm-3m_EaH_0" to phase_names in Run1st_create_gpaw_workflow.py (its POSCAR is already in examples/MgO/CompetingPhases/), and document entries="CompetingPhases_GPAW".
  2. Two of the four new tests replace gpaw.GPAW with a fake.
    • The fake has .scf.converged and .close(), which GPAW 26.7's calculator doesn't have, so the code paths that actually run are untested. This makes the tests pass despite point 1 and 3 here, which should cause test failures with real tests.
    • _In general, we strongly discourage mocked tests in doped, as they often don't properly test the code and miss real failures like this. Wherever possible, the tests should rely on calculations run directly (with GPAW) if quick enough, or to use real DFT/quantum-mechanical data (from GPAW in this context, but we have the same approach with doped/QE/FHI-aims etc).
    • Real calculations are cheap here: about 1.3–1.8 s each for MgO, Mg and O2.
    • Charge rejection tests can use the shipped tests/data/gpaw/MgO/v_Mg_-2 (charge −2).
  3. With several .gpw files in one phase folder, the analyzer picks the wrong one.
    • With relaxed.gpw + singlepoint.gpw, the analyzer parses singlepoint.gpw, while DefectsParser uses relaxed.gpw.
    • A singlepoint.gpw.bak gets picked instead of the real file, which loses the whole phase (No elemental reference ... ['Mg']).
    • Cause: the generic file picker's sort key (utils.py:73) never matches ".gpw", so the order is plain reverse-alphabetical, and substring matching admits .gpw.bak.
    • The "Multiple .gpw files" warning also describes electrostatic potentials and charge corrections, which is wrong here.
    • Two fixes, each tested on a patched copy:
      • match the extension at the end of the filename (a few lines; fixes the .bak loss only; no other caller is affected);
      • or a find_calc_output = _find_gpaw_output hook on the GPAW backend (fixes both):
...
         if calc_output_path and os.path.exists(calc_output_path):
+            # optional backend file selection (e.g. GPAW filename priority, matching defect parsing):
+            if (find_calc_output := getattr(self._backend, "find_calc_output", None)) is not None:
+                with contextlib.suppress(FileNotFoundError, ValueError):  # no/ambiguous: keep generic pick
+                    calc_output_path = find_calc_output(directory)
...

and

+find_calc_output = _find_gpaw_output  # used by CompetingPhasesAnalyzer for file selection
 CALC_OUTPUT_MASK = (".gpw",)
 ...
  1. Phases computed with different settings are mixed with no warning. GPAW has no settings-compatibility check, so the default check_compatibility=True does nothing. Measured shifts in the chemical-potential range, all with zero warnings:

    • Mg with LDA instead of PBE: 0.26 eV
    • Mg with different PAW setups: 0.09 eV
    • O2 in FD mode: about 5 eV

    The pieces already exist in the module (_COMPATIBILITY_PARAMETERS, _comparable, data["gpaw_parameters"]), so about 20 lines would add the check. Leave out kpts/h/gpts, which legitimately differ between phases.

  2. A restart without stored results silently re-runs the SCF inside the parser. If the re-run then fails, the analyzer's warning shows an empty reason (the exception's message is '').

    • The converged_electronic branch never runs: GPAW 26 has no .scf, and 25.7 always reports converged.
    • Fix: a one-line guard, checked on 25.7 and 26.7:

if calc.calculation_required(calc.atoms, ["energy"]):
raise ValueError(f"{gpw_file} holds no converged energy")

get_calculation_outputs has the same pre-existing behaviour, so a shared helper would fix both.
6. The k-point mesh is only recorded for {"size": ...} settings. For the example's {"density": 3.0} Mg phases it shows "N/A" (GPAW actually used 7×7×4). This matches GPAW's own log for every form:

[get_monkhorst_pack_size_and_offset(calc.get_bz_k_points())[0].tolist()]
7. Docs and example consistency.

  • a. Run2nd_run_and_analyze.py still uses the old manual route (about 3 s per file versus 0.05 s).
  • b. processes=1 is unnecessary; the default already parses .gpw files serially.
  1. Simplifications and nits.
    • Parameters are stored twice (entry.parameters and data["gpaw_parameters"]).
    • The subfolder argument is never passed by doped.
    • Docstring lines at 415, 418 and 419 exceed 79 characters, the argument layout differs from doped's style, and one comment mixes singular and plural.

@kavanase
kavanase force-pushed the feat/gpaw-competing-phase-parsing branch from 442df07 to c85a5bd Compare October 31, 2026 14:40
@SMTG-Bham SMTG-Bham deleted a comment from coderabbitai Bot Oct 31, 2026
@SMTG-Bham SMTG-Bham deleted a comment from coderabbitai Bot Oct 31, 2026
@kavanase

Copy link
Copy Markdown
Member

Note that I rebased to the latest IO_refactor, which includes the updates I mentioned in #156

@sblisesivdin

Copy link
Copy Markdown
Author

You're right, and sorry for overlooking your earlier guidance on mocked tests in #149. I've replaced the fake calculators with real GPAW calculations and the shipped restart files. The tests now cover the documented example layout and the failures you identified. Thanks for catching this again.

…ses` and note (don't need to worry about charged competing phase calculations)
…e GPAW competing-phase branch

Both sides branched from c85a5bd. Kept from this branch: the light `read_gpw`/ULM reader,
`_open_gpaw_restart` (stored-results check for all four `.gpw` readers), the charge check removal, and
the example's primitive-cell host, Niggli-reduced phases and 400 eV / k-density 4 settings.

Taken from d3bc935: GPAW `check_entry_compatibility` (k-point/grid meshes excluded), the
`find_calc_output` backend hook for analyzer file selection, competing-phase wording in the multiple-files
warning, k-point meshes from `get_bz_k_points()`, `converged_electronic`, no duplicated
`entry.parameters`, no unused `subfolder` argument, the example docstrings and the real-calculation tests
(phase fixture, analyzer, file priority, settings mismatch, k-point mesh and example inputs).

Changed relative to d3bc935:
- `find_calc_output` errors (`ValueError` for ambiguous filenames, as well as `FileNotFoundError`) fall
  back to the generic file pick with a warning, rather than failing the whole analysis.
- The list-mode docstring uses the wording already on IO_refactor (57faf9b), to merge cleanly there.
- Dropped `_read_gpaw_calculation` (superseded by `_open_gpaw_restart`), the charge check test, and
  `test_gpaw_restart_without_energy` (covered by `test_restart_without_results_raises`).
- `pytest.warns` -> `_run_func_and_capture_stdout_warnings`; lazy `ase` k-point import; doped-style
  docstrings; `find_calc_output` documented in Adding_Calculators.rst.
- Remaining review nits: doped-style Args layouts and the singular/plural protocol-name comment.
@coderabbitai

coderabbitai Bot commented Nov 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 42a744c7-de80-4e0e-acde-9346d78b5b21

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@kavanase
kavanase merged commit cb44afd into SMTG-Bham:IO_refactor Nov 1, 2026
1 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants