[wien2k] Store high-symmetry k-path labels in dft_bands_input - #23
[wien2k] Store high-symmetry k-path labels in dft_bands_input#23harrisonlabollita wants to merge 2 commits into
Conversation
Read the high-symmetry point block that dmftproj appends to case.outband and store it as kpts_labels and kpts_labels_idx, the same keys and format as the VASP KPOINTS_OPT conversion. The keys are only written when the block is present, so older files convert unchanged.
There was a problem hiding this comment.
Review of the Wien2k k-path label conversion
Attribution: this review was produced by Claude (Opus 5) running in Claude Code, driven by @the-hampel. Findings below were checked by running code — parsing the shipped
dmftprojFortran and reproducing the parser against syntheticcase.outbandblocks — not by reading the diff alone.
No blockers. The feature is right and the parity claim holds; everything below is a hardening suggestion the author can fold in.
Verified correct
Checked against the Fortran rather than the description:
outband.f:277—WRITE(ouband,'(2i6,a)') i, labels(i)%pos, labels(i)%kname. The block exists and the format is as claimed.read_k_list.f—labels(ilab)%posis 1-based intonkband, andnkbandis the first value written tocase.outband(outband.f:56), which is exactly then_kthe converter reads back. So both thepos - 1shift and theidx[-1] >= n_kbound are correct.knamecarries a trailing blank:i = INDEX(buf,' ')followed bykname(1:i) = buf(1:i)captures the name plus its separator, and theaedit descriptor then writes the full declared length. The regex's\S*handles this correctly.- Both
exceptclauses inconvert_bands_inputre-raise, son_kis always bound by the time_read_kpath_labelsis called — no latentNameErroron a corrupt file. - The new
### Wien2kChangeLog section is correctly placed.
The parser is more fragile than it needs to be
Reproduced the following against synthetic blocks written in exact (2i6,a), including the trailing blank in kname:
| case | result |
|---|---|
nominal, n_k = 501 |
['GAMMA','X','W','L','GAMMA','K'] at [0,121,181,267,372,500] — matches the PR's stated verification |
a label not starting with [A-Za-z] (e.g. XCrySDen's \xG for Gamma) |
silently returns ['K'] at [500] — passes validation, writes a single tick |
| one malformed line in the middle of the block | silently returns only the tail, ['L','GAMMA','K'] at [267,372,500] — passes validation |
| two labels on the same k-point (discontinuous path, `X | Y`) |
The backward walk breaks on the first non-matching line, so any interruption truncates the front of the block — and a front-truncated block is still strictly ascending and still inside [0, n_k), so the existing guard never fires. The failure is silent in both middle cases.
Two cheap fixes, both suggested by the code as written:
- Use the counter that is already being parsed.
match.group(1)isdmftproj's running indexiand is currently captured and discarded. If the first parsed line's counter is not1, the walk was truncated — warn and skip, exactly as the ascending check does. That catches both silent-truncation rows above for free, with no new parsing. - Parse by column rather than by regex. The docstring already states the format is fixed
(2i6,a): columns 1-6 counter, 7-12 position, 13+ label. Slicing those is exact and drops the[A-Za-z]charset assumption entirely. As a smaller version of the same fix, loosening the label group to(\S+)would handle the\xGcase on its own. Worth noting too that the current regex requires whitespace between the two integer fields, which quietly assumespos < 100000— harmless in practice, but it is an assumption the column form does not need to make.
Testing
Please do add the test that is offered in the PR description. The parity argument cuts both ways: VASP got the feature and test/python/vasp/converter/test_converter_svo_bands.py, which asserts both keys and their exact values.
test/python/wien2k/ has no .outband fixture and a real one is large, so the pragmatic route is to promote _read_kpath_labels from a nested function to a module-level or @staticmethod helper and unit-test it on a twenty-line synthetic tail covering the four rows in the table above. No Wien2k installation and no large fixture required, and it locks in the pos - 1 convention that the whole feature rests on.
Minor
- Validation asymmetry. Wien2k now checks strictly-ascending and in-range; VASP checks neither. If the goal is a uniform contract for plotting code, either lift the check into a helper both converters call, or document in
h5structure.rstthat consumers cannot rely on the invariant. readlines()slurps all ofcase.outband— tens of MB once projectors are in it — to read roughly six trailing lines. Not a new cost, sinceread_fortran_filealready walks the whole file, but a seek-from-end would be easy here.- Drive-by, pre-existing: the ChangeLog entry "Read VASP
ICHARG=5miscellaneous input fromvaspout.h5" sits under ### Wannier90. Since this PR touchesdoc/ChangeLog.mdanyway, it would be a natural place to move it.
|
Thanks for the review! All five points are addressed in 37cbfc0. ParserI went with both suggested fixes. Column parsing. The block is now sliced as the fixed The counter. We use the counter do two things:
Together those turn the two silently-truncating rows of your table into a warning and Discontinuous paths. Relaxed the ascending check from Tail read. TestingAdded Validation asymmetryAdded documentation rather than the shared helper. The two ChangeLogMoved the Review comments
|
the-hampel
left a comment
There was a problem hiding this comment.
Approving — the hardening commit addresses everything from my earlier review, and then some.
Attribution: as with the previous review, the verification below was carried out by Claude (Opus 5) running in Claude Code, driven by @the-hampel.
I re-ran the four failure cases from my earlier comment against 37cbfc0:
| case | before | after |
|---|---|---|
nominal, n_k = 501 |
correct | correct, unchanged |
label not starting with [A-Za-z] (XCrySDen's \xG) |
silently returned ['K'] |
['\xG', 'X', 'W', 'L', '\xG', 'K'] |
| malformed line in the middle of the block | silently returned only the tail | warns and skips |
two labels on the same k-point (X|Y) |
dropped all labels | both ticks kept |
The changes go a little further than what I suggested, all in the right direction:
- column slicing in
_parse_kpath_label_lineinstead of the regex, matching the(2i6,a)the docstring already documented; - the previously discarded counter now used as the integrity check, with an early
breakat counter 1 so a(2i6,a)-parseable line sitting ahead of the block can never be reached; - the helper promoted to module level, which is what makes it unit-testable;
readlines()replaced by an 8 KB seek-from-end tail read, with the partial first line dropped;diff <= 0relaxed todiff < 0, so a discontinuous path no longer discards the whole block.
test_kpath_labels.py is a good test and passes: one synthetic block exercising all four cases at once, 2000 padding lines so the tail-read truncation and partial-first-line paths are actually hit, and a decoy parseable line in front of the block. Registered as Py_wien2k_kpath_labels. Py_wien2k_convert still passes, and dropping import re is safe — no other use in the file.
Nothing further from me.
dmftproj -bandappends the high-symmetry points of the band path to the end ofcase.outband, but the Wien2k converter stopped reading before them, so adft_bands_inputgroup from Wien2k carried no way to place or name the ticksof a band plot without reading this file. VASP gained exactly this in PR #3 (
kpts_labels/kpts_labels_idx, read from/input/kpoints_opt); this PR does the same forWien2k, writing the same two keys in the same format so plotting code does not
have to branch on the DFT code.
Feature
convert_bands_input()gains a_read_kpath_labels()helper with the samecontract as its VASP counterpart: it returns
(labels, idx), or(None, None)when no label data is available, and the two keys are only written in the
former case. Old
case.outbandfiles without the block therefore convertunchanged.
Implementation details
The block has no header —
dmftprojwrites one line per high-symmetry point infixed Fortran format
(2i6,a)after the projector data:so it is found by walking backwards from EOF over the trailing run of
counter index labellines. Two details worth mentioning:split(), because the labelabuts the integer field —
122Xsplits into two tokens, not three.the VASP convention.
[0, n_k); a block failing that warns viampi.reportand is skipped ratherthan writing bad tick positions into the archive.
Verification
Checked against a Wien2k
case.outbandwithn_k = 501, giving['GAMMA', 'X', 'W', 'L', 'GAMMA', 'K']at[0, 121, 181, 267, 372, 500].No test is included:
test/python/wien2k/. Happy to add a trimmed-down oneif reviewers would rather have the coverage.
Docs
dft_bands_inputindoc/h5structure.rstdocuments the two keys, which PR #3added to the archive but not to the reference page.
🤖 Generated with Claude Code