Skip to content
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

Closed
IAlibay opened this issue Aug 17, 2019 · 3 comments · Fixed by #2339 or #3294
Closed

Improving the NCDFWriter behaviour (units + scale_factors) #2327

IAlibay opened this issue Aug 17, 2019 · 3 comments · Fixed by #2339 or #3294

Comments

@IAlibay
Copy link
Member

IAlibay commented Aug 17, 2019

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:

  • The writer currently writes out cell_angles in units of degrees unlike the convention agreed degree.
  • The writer currently writes out 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 a scale_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:

  • Change the units of cell_angles to degree, deprecate the reading of degrees and remove it from MDAnalysis 1.0 onwards. (Addressed by Changes cell_angle units to degree (Issue #2327) #2339 )
  • Default the writing style to that of trajectories written by the AMBER MD engines (or at least pmemd as of Amber 18 + AmberTools 19).
  • Implement an option for the writer to pick up whatever 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.

orbeckst pushed a commit that referenced this issue Aug 19, 2019
…#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)
@orbeckst
Copy link
Member

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 .

@IAlibay
Copy link
Member Author

IAlibay commented Mar 10, 2021

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).

@orbeckst
Copy link
Member

I agree that we should have a warning 1.0.2 — if nothing else we can then say that we warned everyone.

@IAlibay IAlibay mentioned this issue Mar 14, 2021
9 tasks
@IAlibay IAlibay modified the milestones: 1.1.0, 2.0 Mar 24, 2021
IAlibay added a commit that referenced this issue Mar 27, 2021
Towards #2327 

# Work done in this PR
* Adds warning for upcoming changes to scale_factor writing in NCDFWriter
zemanj pushed a commit to zemanj/mdanalysis that referenced this issue Apr 3, 2021
Towards MDAnalysis#2327 

# Work done in this PR
* Adds warning for upcoming changes to scale_factor writing in NCDFWriter
lilyminium added a commit that referenced this issue Aug 20, 2021
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants