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

Finalizing #2537 #2616

Merged
merged 3 commits into from
Mar 15, 2020
Merged

Finalizing #2537 #2616

merged 3 commits into from
Mar 15, 2020

Conversation

wvdtoorn
Copy link
Contributor

@wvdtoorn wvdtoorn commented Mar 12, 2020

Fixes #2502

Changes made in this Pull Request:

  • [DOC] add sphinx deprecated paragraph to density/BFactor2RMSF docstring
  • [TEST] decrease precision in assert_almost_equal test_density/test_density_from_PDB
  • [TEST] Bring back TestGridImport class in test_density

PR Checklist

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

- [DOC] add sphinx deprecated paragraph to density/BFactor2RMSF docstring
- [TEST] decrease precision in assert_almost_equal test_density/test_density_from_PDB
- [TEST] Bring back TestGridImport class in test_density
- [DOC] Update CHANGELOG
@wvdtoorn wvdtoorn marked this pull request as ready for review March 12, 2020 11:15
@wvdtoorn
Copy link
Contributor Author

I see my editor had a go at some trailing white spaces in the CHANGELOG, let me know if that's a problem for you.

@codecov
Copy link

codecov bot commented Mar 12, 2020

Codecov Report

Merging #2616 into densityanalysis-2502 will increase coverage by 0.66%.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           densityanalysis-2502    #2616      +/-   ##
========================================================
+ Coverage                 91.01%   91.68%   +0.66%     
========================================================
  Files                       170       15     -155     
  Lines                     23192     2008   -21184     
  Branches                   2946        0    -2946     
========================================================
- Hits                      21108     1841   -19267     
+ Misses                     1495      167    -1328     
+ Partials                    589        0     -589     
Impacted Files Coverage Δ
__init__.py 91.89% <0.00%> (-2.34%) ⬇️
coordinates/__init__.py 100.00% <0.00%> (ø)
package/MDAnalysis/coordinates/DLPoly.py
package/MDAnalysis/coordinates/TRJ.py
package/MDAnalysis/topology/tpr/obj.py
package/MDAnalysis/analysis/pca.py
package/MDAnalysis/selections/base.py
package/MDAnalysis/topology/ParmEdParser.py
package/MDAnalysis/analysis/encore/utils.py
...ge/MDAnalysis/transformations/positionaveraging.py
... and 148 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 1154532...5ea74a8. Read the comment docs.

@IAlibay
Copy link
Member

IAlibay commented Mar 12, 2020

@orbeckst do you prefer this be a two stage PR (i.e. PR into your branch, and then we close your PR), or for @wvandertoorn's PR to directly go into develop?

@orbeckst
Copy link
Member

Merge into my original PR.

@orbeckst orbeckst requested a review from IAlibay March 12, 2020 19:57
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up :)

Remember to add yourself to AUTHORS too.

Main change required here is to add a bit more details to the .. deprecated docs. Could you also add details for the .. deprecated in notwithin_coordinates_factory? Saying that it will be removed in 2.0.0 with density_from_Universe?

package/CHANGELOG Show resolved Hide resolved
package/MDAnalysis/analysis/density.py Show resolved Hide resolved
package/CHANGELOG Show resolved Hide resolved
- [DOC] Update AUTHORS
- [DOC] More detail on deprecation notice
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

One new change, one info request on a previously requested change.

package/MDAnalysis/analysis/density.py Show resolved Hide resolved
package/MDAnalysis/analysis/density.py Outdated Show resolved Hide resolved
- [DOC] Fix formatting
- [DOC] Fix deprecation notice mix up
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

lgtm thanks for your work @wvandertoorn

I'm still not 100% sure this will end up fixing all the Travis failures we were seeing in #2537, but unfortunately it looks like we can't test this until this gets pushed to @orbeckst's PR.

I'll wait for appveyor to return green here and then squash-merge. If the issues with Travis continue, I might ping you for another PR if that's ok with you?

@IAlibay IAlibay merged commit 837c2de into MDAnalysis:densityanalysis-2502 Mar 15, 2020
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.

4 participants