Fix DFTB+ Hessian parsing and harden coordinate/frequency readers - #47
Merged
Conversation
DFTB+ writes hessian.out as a flat stream of second derivatives wrapped at a fixed number of values per line (with a ragged final line per matrix row), not as a square grid. Routing it through PQAnalysis.read_hessian_file (which requires an already-square np.loadtxt result) raised on every real file: uniform-width output loaded as (n, 4) and failed the square check, and ragged output made np.loadtxt itself fail. Restore flat-token parsing that reads all values and reshapes to (3N, 3N), keeping the explicit size validation. Add a regression test using the real ragged wrapped layout. read_xyz routed coordinates through XYZFrameReader, which stores positions as float32, perturbing every value by ~1e-7 relative to the source and to read_gen (which stays float64). Read the coordinate columns in float64 while keeping XYZFrameReader for validation, atom count, symbols, and cell detection. read_xyz also masked a missing file as 'Invalid XYZ coordinate file.'; move the file read out of the parse guard so a missing file raises FileNotFoundError.
The Hessian.read -> Modes -> wave-number path was only exercised by the integration tests that require the dftb+/modes executables, so it was skipped in CI. Add small fixtures that reproduce the authentic DFTB+ file layouts (a wrapped, ragged hessian.out, a frequencies-only vibrations.tag, and a matching geo_opt.gen) and an integration test that mocks only the modes binary and drives Modes through write -> calculate -> read end to end, asserting the parsed wave numbers and that Hessian.read parses the same ragged layout. The test guards the fixture against being reduced to a square grid by asserting numpy.loadtxt cannot parse it.
Modes.read flattened every line after the header and cast all of it to float, so a vibrations.tag that carries further tag sections after the frequencies (e.g. 'saved_modes :integer:..') crashed on the section header. Read frequency values line by line and stop at the first non-numeric line instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 93.61% 93.63% +0.01%
==========================================
Files 21 21
Lines 1191 1194 +3
==========================================
+ Hits 1115 1118 +3
Misses 76 76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
Validated end to end against the real DFTB+ binaries (DFTB+ 24.1 + MODES 0.03, 3ob-3-1 Slater-Koster set), not just mocks. Full suite with the binaries enabled: 139 passed, 0 skipped (the five integration tests that normally skip without dftb+/modes now run: real geometry optimization, real second-derivatives Hessian, real modes frequencies, and the full thermo pipeline). Concrete evidence on freshly generated output for water:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #46 (shared parsing via PQAnalysis). Verifying that refactor surfaced one critical regression plus a couple of lower-severity issues, fixed here with regression coverage.
Critical: Hessian reading was broken on real DFTB+ output
DFTB+ writes
hessian.outas a flat stream of second derivatives wrapped at a fixed number of values per line (with a ragged final line per matrix row), not as a square grid. The refactor routed it through a reader that requires an already-squarenumpy.loadtxtresult, so it raised on every real file:(n, 4)and failed the square check;numpy.loadtxtitself fail.This is invisible to CI because the integration tests need the
dftb+/modesbinaries and are skipped, while the unit test fed a synthetic square grid DFTB+ never emits. Restored flat-token parsing that reads every value and reshapes to(3N, 3N), keeping the explicit size validation. Verified bit-for-bit against real 72x72 and 78x78 Hessians.xyz coordinate precision
read_xyzrouted coordinates throughXYZFrameReader, which stores positions as float32, perturbing every value by ~1e-7 relative to the source and toread_gen(float64). Now reads the coordinate columns in float64 while keepingXYZFrameReaderfor validation, atom count, symbols and cell detection.Smaller fixes
read_xyzno longer masks a missing file as "Invalid XYZ coordinate file."; it raisesFileNotFoundErroragain.Modes.readstops at a trailing tag section (e.g.saved_modes :integer:..) instead of casting the section header to float and crashing.Test coverage
numpy.loadtxtcannot parse it, so the fixture can't be reduced to a square grid).tests/data/calculator/modes/) plus an integration test that drivesHessian.read->Modes-> wave numbers end to end with only themodesbinary mocked, closing the no-binary gap.read_xyz; a trailing-section test forModes.read.134 passed, 5 skipped (the 5 still need the DFTB+ binaries); pylint 7.94/10.