Conversation
perf: make VACF, MSD, RDF, and momentum bitwise exact
perf: batch exact file-backed analyses
perf: reduce exact RDF execution overhead
perf: reduce CLI startup overhead
The vibrational input file documents hessian_sign as one of auto, positive, negative, 1 and -1, but neither number worked. The parser reports an unquoted number as an integer, which the string-only key reader rejected, and the sign resolver only recognized the numbers as numbers, so the whitelisted strings "1" and "-1" fell through to its error branch. Read the key as an integer when the parser reports one, and let the sign resolver accept the numeric spellings. All five documented values now select the intended convention; anything else is still rejected.
lark calls terminal callbacks with the token itself, so indexing it returned its first character and any selection index above 9 selected the wrong atom. Read the token as a whole, still accepting a list for direct callers. Closes #173
The positional argument was declared without nargs, so argparse passed a single string where a list is required and every invocation failed. Declare it like the other converters. Closes #175
The exception belonging to an error was raised inside the logging call, which was only reached when the logger was enabled for that level. A higher level therefore turned every guard into a no-op, so that an existing output file was overwritten instead of reported. Move the raising into its own method and call it when the record is not logged. Closes #177
fix: accept the documented numeric hessian_sign values
fix: read whole integer tokens in selections
fix: accept trajectory files on the traj2qmcfc command line
…f-log-level fix: raise logged errors independently of the logging level
The reverse lookup table was built by inverting atomicNumbers with a
dict comprehension. Several symbols map onto the same atomic number, so
the last one written won: atomic number 1 resolved to the dummy element
"dum" rather than hydrogen, and 999 resolved to "x" rather than "q".
Constructing an element from its atomic number therefore produced the
wrong species. Element(1) was not equal to Element("H") and carried a
mass of 1.0 instead of 1.00794, so a water molecule built as
Atom("O1", 8) plus two Atom("H1", 1) weighed 17.9994 amu instead of
18.01528 amu and its centre of mass was displaced accordingly. An
elem(1) selection matched no atoms at all.
Build the table by iterating in insertion order and keeping the first
symbol seen for each atomic number, which is the canonical one.
The exact batch estimators size their position and velocity buffers from the frame count reported by the trajectory reader and then fill them from the frame generator without checking how many frames actually arrived. A trajectory whose counted frame number exceeds the number of frames the parser yields, for instance an xyz file with surplus blank lines between frames whose surplus happens to be a multiple of the frame size, leaves the trailing rows of the np.empty buffers uninitialised, and those rows are passed straight to the kernels. The analyses then returned NaN or arbitrary numbers, or aborted with a misleading zero-norm error, while the command line tools still exited successfully and wrote the result to the output file. Both batch paths now compare the number of frames they received against the expected one and raise MSDError or VACFError on a mismatch, so a miscounted trajectory fails loudly instead of producing meaningless diffusion coefficients or correlation functions.
The -n and --n-molecules aliases were passed to add_argument as a single comma-joined string, so argparse registered one malformed option. The help text advertised --n-molecules, but using it failed with unrecognized arguments and only -n worked via prefix matching. Splitting the aliases into separate option strings makes both work.
The --periodic option declared choices=[True, False, None] without a type converter, so argparse compared the raw command-line string against Python objects and rejected every value, including the ones its own error message suggested. The option could therefore never hold anything but its None default. A converter now maps the strings True, False and None (case-insensitive) to the corresponding Python values and reports a proper error for anything else.
The --progress option was registered with store_false, so passing it disabled the progress bar although its help reads "Show progress bar." It now uses BooleanOptionalAction: --progress shows the bar, --no-progress hides it, and the default stays enabled. The RDF module also copied config.with_progress_bar into its namespace at import time, before the CLI parses arguments, so the flag never reached the two tqdm loops in the analysis. It now reads the config attribute at call time like msd, vacf and momentum do.
Cell built its box matrix without any validation, so a zero-length box escaped as a raw numpy.linalg.LinAlgError from the inverse-matrix setter, far from the offending input, and geometrically impossible angle triples produced a NaN box matrix that silently corrupted volumes, imaging and downstream analysis output. setup_box_matrix now rejects non-positive box lengths, angles outside (0, 180) degrees and angle triples whose box-matrix radicand is not positive, raising a new CellError that names the offending values. Valid cells, including the vacuum cell, are unaffected.
TrajectoryReader.read() unconditionally assigned its optional topology argument to self.topology, so calling read() without an argument reset the topology passed to the constructor back to None. As a consequence, read_trajectory(file, topology=...) silently ignored the given topology and rebuilt a guessed one from the file, replacing user-supplied atom types and residue ids. read() now only overrides the stored topology when one is explicitly given.
fix: resolve atomic numbers to the canonical element symbol
fix: detect short frame generators in the batch analysis paths
fix: validate box lengths and angles on Cell construction
fix: keep the constructor topology in TrajectoryReader.read
fix: make the progress flag match its help text
fix: register -n and --n-molecules as separate option strings
fix: parse xyz2gen --periodic values from the command line
The WORD terminal did not allow '/', so a value like qm_script_full_path = /path/to/script; failed to parse, and unbracketed comma-separated lists like inner_region_center = 0,3,6; were not part of the grammar at all. continue_input died on both with a lark UnexpectedCharacters error before writing any output. WORD now accepts '/', and the PQ grammar accepts an unbracketed comma-separated list of primitives as a value, transformed like a bracketed array. The rule is limited to the PQ grammar so QMCFC selector lists keep their string element type. Fixes #216.
fix: parse paths and unbracketed lists in PQ input files
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 @@
## main #218 +/- ##
==========================================
+ Coverage 93.32% 94.02% +0.69%
==========================================
Files 175 177 +2
Lines 8588 9259 +671
==========================================
+ Hits 8015 8706 +691
+ Misses 573 553 -20
🚀 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.
Release v1.6.0.
Features since v1.5.0: extxyz trajectory conversion and output profiles, moltype inference for xyz2rst, faster CLI startup and batched file-backed analyses. Fixes: input file parsing of paths and unbracketed lists (#216), selection index parsing, traj2qmcfc file arguments, log-level independent error raising, cell validation, uninitialised analysis buffers, element lookup by atomic number.