fix: read the cells of extended xyz trajectories - #183
Open
galjos wants to merge 1 commit into
Open
Conversation
The cell generator behind TrajectoryReader.cells parsed every header line as a PQ xyz header of the form "n_atoms a b c alpha beta gamma", regardless of the trajectory format. An extended xyz frame stores the box as Lattice metadata in the comment line and puts only the atom count in the header line, so every frame matched the single-field case and yielded a vacuum cell while the comment line was skipped together with the atoms. Reading the same file with read() returned the correct box, so the two entry points disagreed. Any analysis that takes the cells from the reader instead of from the frames therefore worked with an infinite volume: an RDF over an extended xyz trajectory produced an average volume of inf and wrote a column of nan and inf, with an exit status of zero and nothing but a numpy warning to indicate that the result was meaningless. The generator now dispatches on the trajectory format and reads the Lattice metadata for extended xyz files, carrying the previous cell over to frames that define no Lattice, exactly as the frame generator does. A format for which no cell layout is known raises instead of silently reporting vacuum. The xyz branch also takes the atom count from the header of each frame rather than from the first line of the first file, so that a set of files with differing atom counts no longer runs out of step.
Contributor
|
PYLINT REPORT Your code has been rated at 9.78/10 Full reportRaw metrics
Duplication
Messages by category
% errors / warnings by module
Messages
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #183 +/- ##
==========================================
+ Coverage 93.86% 93.92% +0.05%
==========================================
Files 177 177
Lines 9215 9254 +39
==========================================
+ Hits 8650 8692 +42
+ Misses 565 562 -3
🚀 New features to boost your workflow:
|
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
1 participant
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.
TrajectoryReader.cellsparsed every header line as a PQ xyz header regardless of the trajectory format. An extended xyz header holds only the atom count, so every frame produced a vacuum cell and the box in theLatticemetadata was never read.RDF takes the average volume from
cells, so running it on an extended xyz trajectory produced an infinite density and wrote nan and inf into g(r), while the same trajectory read into memory gave correct values.The cell scan is now format aware and reads each frame own atom count, which also fixes variable size extended xyz files, where the previous code reused the first count and drifted onto atom lines. A format without a cell scanner raises instead of returning vacuum cells.
Present in v1.5.0. 949 tests pass in both type checking modes.