-
Notifications
You must be signed in to change notification settings - Fork 663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving the NCDFWriter behaviour (units + scale_factors) #2327
Comments
…#2326) * Fixes #2323 * Now checks for presence of scale_factor in all AMBER NetCDF convention variables * Adds support for scale_factor values, set in NCDFReader __init__ and multiplied through in _read_frame. * assumes/uses AKMA velocity scaling by default (as produced by AMBER) * Implements several new exception checks, mainly revolving around ensuring that units are properly adhered to. * Also included in this PR is part framework for the introduction of an NCRST file reader (singleframe version of the NCDFReader for .ncrst files). These can be seen in the _NCDFGenerator class of test_netcdf.py * Update changelog * DeprecationWarning (1.0) if NCDF file uses units "degrees" instead of correct "degree" for attribute "cell_angle" (MDA < 0.20.0 would write degrees (see #2327), we will now correct these units and from 1.0 onwards raise an error if we encounter degrees instead of degree)
I labeled it as defect because "degrees" is wrong. The other solutions are better choices than what we have so far. The reading of degrees was already deprecated in PR #2326 . |
We probably should fix this for for 2.0. In the interest of changing the way that velocities are written, do we think it's worth adding a warning in 1.0.2 that the velocity writing behaviour is going to change? (I'm reluctant to just add all the warnings, but at the same time doing this without warning is probably a bad idea). |
I agree that we should have a warning 1.0.2 — if nothing else we can then say that we warned everyone. |
Towards #2327 # Work done in this PR * Adds warning for upcoming changes to scale_factor writing in NCDFWriter
Towards MDAnalysis#2327 # Work done in this PR * Adds warning for upcoming changes to scale_factor writing in NCDFWriter
- Fixes #2327 - Completes #3185 - Adds `scale_factor` writing support to NCDFWriter. - Changes the default `scale_factor` value for velocity writing to 20.455 in order to match the AMBER default Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com> Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Is your feature request related to a problem? Please describe.
A discussed in PR #2326, improving the NCDFReader has highlighted the fact that the NCDFWriter probably needs to be updated to ensure consistency with the AMBER NCDF convention (http://ambermd.org/netcdf/nctraj.xhtml) and the way AMBER MD engines write out certain variables by default.
Notably the following questions/issues have been raised:
cell_angles
in units ofdegrees
unlike the convention agreeddegree
.velocities
directly in whatever values MDAnalysis holds (usually units of Angstrom/ps). However, the standard behaviour of the AMBER MD engines appears to be to store the values in Angstrom/AKMA time units with ascale_factor
of 20.455 in order to yield Angstrom/ps values when multiplied through. Assuming all readers properly implement the AMBER NetCDF convention and/or display scaled values to users, this should not be an issue. However in practice this is not actually the case (see this conversation for an example where things can easily become confusing: http://archive.ambermd.org/201908/0067.html).Describe the solution you'd like
As discussed with @orbeckst the current plan is to:
cell_angles
todegree
, deprecate the reading ofdegrees
and remove it from MDAnalysis 1.0 onwards. (Addressed by Changes cell_angle units to degree (Issue #2327) #2339 )scale_factors
were used on read, and allow for user-defined values too.Additional context
Work on a PR related to this will begin after #2326.
The text was updated successfully, but these errors were encountered: