Skip to content

[test] Add spin-orbit + spin-polarized Wien2k converter test (CaOs2) - #4

Open
krystophny wants to merge 1 commit into
TRIQS:unstablefrom
krystophny:port/dmftproj-soc-harness
Open

[test] Add spin-orbit + spin-polarized Wien2k converter test (CaOs2)#4
krystophny wants to merge 1 commit into
TRIQS:unstablefrom
krystophny:port/dmftproj-soc-harness

Conversation

@krystophny

@krystophny krystophny commented Jun 15, 2026

Copy link
Copy Markdown

Part of #7 (dmftproj Fortran-to-Python port).

Adds the first spin-orbit + spin-polarized golden test for the Wien2k
converter. dftkit currently only tests the non-SOC SrVO3 case, so the SOC path
of dmftproj and the converter (the combined-spin ud block, time-reversal
symmetry operations) is untested.

CaOs2 is a cubic fluorite-type cell with two symmetry-equivalent correlated Os
atoms; its magnetic point group has 16 operations, 8 of them time-reversal. The
test runs convert_dft_input on a small (8 k-point) dmftproj output fixture and
h5diffs against a reference produced by this converter.

It is also the characterization harness for incrementally porting
fortran/dmftproj to Python: with the converter output locked by this golden
test, each ported stage can be proven to leave the result unchanged.

Verification

The reference wien2k_soc_convert.ref.h5 was generated with this converter
(triqs_dftkit.wien2k.Converter.convert_dft_input); the test reruns the same
converter and asserts h5diff == 0. CI builds dftkit and runs it via ctest.

…CaOs2)

dftkit's Wien2k converter is only covered by the non-SOC SrVO3 case. This adds a
SOC + spin-polarized golden test: CaOs2, a cubic fluorite cell with two
symmetry-equivalent correlated Os atoms whose magnetic point group has 16
operations, 8 of them time-reversal. It exercises the combined-spin 'ud' block
and the time-reversal symmetry path of dmftproj + the converter on a small
8-k-point fixture, validated by h5diff against a reference produced by this
converter.

@the-hampel the-hampel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed as the first step of the #7 stack (#4#5#8#9#10#11#12#15#16#13/#19). Thanks for laying the harness down first — locking convert_dft_input before touching fortran/dmftproj is the right order, and the SOC/spin-polarized gap in the test suite is real.

Verification done locally

Rebased port/dmftproj-soc-harness onto current origin/unstable (20e6373) — conflict-free, delta unchanged at +1497 over 9 files. Configured and built in a fresh build directory and ran the full suite:

20/20 tests passed
 2/20 Test #2: Py_wien2k_soc_convert ... Passed  0.85 sec

(For anyone reproducing this: an existing build/ that has previously been configured on another branch can leave stray test_*.py in test/python/vasp/converter/, which run_suite.py then picks up and fails on. That is a stale-build-dir artifact, not this PR.)

Substantive: the fixture's projectors are not orthonormal

This is the one I'd like to resolve before the stack builds on it.

The fixture declares two correlated Os shells of dim = 10 each — 20 spin-orbitals — but the band window is bands 60–76, i.e. 17 bands, at a single k-point. 20 > 17, so the correlated subspace cannot fit in the window and dmftproj's Löwdin orthonormalization cannot succeed. Reading the shipped reference back:

per shell:            max |P P† − 1|          = 4.98e-01
stacked 20x17 block:  eig(P P†)               = {0 x5, 1 x15}   -> rank 15
                      trace(P P†)             = 15.000

For contrast, the same quantity on the existing SrVO3 reference is 2e-8 at every one of its 10 k-points, rank-full at each.

So five of the twenty correlated directions are exactly null. As a pure converter characterization test that is survivable — the converter is reshaping and I/O, and it will reshape a rank-deficient block as faithfully as a good one. But the PR sells this as the harness that proves the port "leaves the result unchanged", and #7 itself says the .ctqmcout orthonormalization "fixes the numerical contract for everything downstream". Pinning that contract on a singular overlap matrix is the worst place to pin it: eigenvector choice in the null space is arbitrary, so a Python Löwdin can legitimately differ from the Fortran one there for reasons that say nothing about correctness — and conversely a genuine regression in the well-conditioned part can hide behind it.

Concretely I'd suggest either widening the energy window in case.indmftpr until the window holds at least 20 bands and regenerating the fixture, or dropping to one correlated Os atom if the two-atom permutation symmetry is not what you're after. Ideally the test then also asserts orthonormality directly, so the harness fails loudly rather than silently locking a degenerate case:

P = ar['dft_input']['proj_mat'][ik, 0, icrsh, :dim, :n_orb]
assert np.abs(P @ P.conj().T - np.eye(dim)).max() < 1e-6

Also worth fixing

n_k = 1, not 8. The description says "a small (8 k-point) dmftproj output fixture". CaOs2.ctqmcout line 2 is 1, and the reference has n_k = 1, bz_weights = [1.0]. So no k-dependence is locked at all, which matters for a harness meant to catch a per-k projector regression. (The other half of the claim is accurate: 16 symmetry operations with time_inv = [0]*8 + [1]*8, and 48 rot_symmetries from .outputs.) Either fix the sentence or, better, ship a few k-points.

Reference provenance is undocumented. The fixture cannot be regenerated from what is in the tree: case.indmftpr and case.almblm* only arrive in #5 and later. Since this reference governs the whole port, a short header comment or README recording the Wien2k version, the init_lapw / x lapw1 -so / x lapwso / dmftproj invocations, the k-mesh and the window would pay for itself many times over.

h5diff tolerance is loose. h5diff(a, b) defaults to precision=1e-6. For a characterization test whose whole job is to show a reimplementation is a no-op — reading the same fixture bytes, so cross-platform drift is at 1e-15 — that is about six orders of magnitude of slack. Please pass an explicit tight tolerance.

CaOs2.oubwindn is dead. It is byte-identical to CaOs2.oubwinup (same blob), and for SP == 1 and SO == 1 convert_misc_input takes up and never looks at dn. Drop it, or make it non-identical if the intent is to cover the dn fallback.

Nits

  • wien2k_soc_convert.ref.h5 in the file(COPY …) list is redundant: test/python/CMakeLists.txt already GLOB_RECURSE-copies every *.h5 into the binary tree. The SrVO3 block right above correctly omits its own reference — worth matching.
  • The 2011 GPL boilerplate header on a new 2026 file is copied from wien2k_convert.py; recently added tests in this repo (e.g. test/python/vasp/converter/test_converter_svo_bands.py) carry none.
  • from h5 import * is unused in wien2k_soc_convert.py.
  • Fixtures are shipped uncompressed here while the later PRs in the stack gzip them; consistency would be nice, though 630 KB is not a problem on its own.

Note for the rest of the stack

The test only calls convert_dft_input. convert_parproj_input and convert_transport_input are never exercised, so dft_parproj_input and dft_symmpar_input stay unlocked for the SOC path — which is exactly what #10 and #11 port. Worth extending the harness here (it needs CaOs2.sympar / CaOs2.parproj fixtures) rather than leaving those two PRs to validate themselves.

None of the "also" or "nits" items block; the orthonormality question does, in the sense that fixing it later means regenerating the reference and rebasing everything above.


Attribution: this review was produced by Claude (Opus 5) running in Claude Code, driven by @the-hampel.

@the-hampel
the-hampel force-pushed the port/dmftproj-soc-harness branch from 830cb84 to 12dbb9b Compare September 10, 2026 09:16
@the-hampel

Copy link
Copy Markdown
Member

Heads-up: I force-pushed the rebase onto current unstable to this branch (830cb8412dbb9b, base now 20e6373). It was a clean replay — the tree is identical, no conflicts — but git pull --rebase / re-fetch before you touch it locally.

Note this rebases only #4; #5 and everything above still carry the old 830cb84 as their base, so they'll each need the same treatment as we work up the stack.

Attribution: this note was produced by Claude (Opus 5) running in Claude Code, driven by @the-hampel.

@the-hampel

Copy link
Copy Markdown
Member

Hi @krystophny ,

First of all sorry for the delay working on this. I simply did not get to this until now. I reviewed the code work now with Claude and would be happy to work through all PR's if you are still available. Please let me know if you have any questions.

Best,
Alex

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