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

Issue 2327 - Warn of upcoming scale_factor changes #3185

Merged
merged 6 commits into from
Mar 27, 2021

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Mar 21, 2021

Towards #2327

Changes made in this Pull Request:

  • Adds FutureWarning & docstring related to the upcoming changes to the way in which we write scale_factor entries with the NCDFWriter.

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #3185 (9151cd3) into master (9a03aec) will increase coverage by 0.84%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3185      +/-   ##
==========================================
+ Coverage   90.96%   91.81%   +0.84%     
==========================================
  Files         162      167       +5     
  Lines       22192    22699     +507     
  Branches     3201     3201              
==========================================
+ Hits        20187    20840     +653     
- Misses       1382     1775     +393     
+ Partials      623       84     -539     
Impacted Files Coverage Δ
package/MDAnalysis/coordinates/TRJ.py 97.17% <100.00%> (+2.61%) ⬆️
package/MDAnalysis/topology/tpr/obj.py 96.82% <0.00%> (-3.18%) ⬇️
...onality_reduction/DimensionalityReductionMethod.py 97.14% <0.00%> (-2.86%) ⬇️
package/MDAnalysis/tests/datafiles.py 31.25% <0.00%> (ø)
package/MDAnalysis/due.py 56.09% <0.00%> (ø)
package/MDAnalysis/analysis/legacy/x3dna.py 0.00% <0.00%> (ø)
package/MDAnalysis/tests/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/analysis/legacy/__init__.py 0.00% <0.00%> (ø)
package/MDAnalysis/core/selection.py 99.52% <0.00%> (+<0.01%) ⬆️
...sis/analysis/encore/clustering/ClusteringMethod.py 96.96% <0.00%> (+0.04%) ⬆️
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a03aec...9151cd3. Read the comment docs.

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @IAlibay. Is there currently a PR open implementing these changes in 2.0?

Also please can you resolve the CHANGELOG conflict :)

@IAlibay
Copy link
Member Author

IAlibay commented Mar 24, 2021

Looks good, thanks @IAlibay. Is there currently a PR open implementing these changes in 2.0?

Also please can you resolve the CHANGELOG conflict :)

Looks good, thanks @IAlibay. Is there currently a PR open implementing these changes in 2.0?

Also please can you resolve the CHANGELOG conflict :)

Not yet, I'll deal with it post-1.1.0 release / dealing with our March deadlines 😉

@lilyminium
Copy link
Member

I can't seem to squash-merge this on the app (merge seems to be the only option). If anyone else feels like doing it, please feel free!

@IAlibay IAlibay merged commit 04e8af4 into MDAnalysis:master Mar 27, 2021
@IAlibay IAlibay deleted the issue-2327-warn branch March 27, 2021 17:18
zemanj pushed a commit to zemanj/mdanalysis that referenced this pull request 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 pull request 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
Development

Successfully merging this pull request may close these issues.

2 participants