new DensityAnalysis #2537

orbeckst · 2020-02-18T08:09:18Z

Changes made in this Pull Request:

new DensityAnalysis (based on @nawtrey 's pmda.density.DensityAnalysis)
added tests (same as for the old density_from_universe() function) so old and new give the same answers
deprecated density_from_universe(), density_from_PDB(), Bfactor2RMSF(), BfactorDensityCreator (remove in 2.0)
added superficial tests for density_from_PDB() and Bfactor2RMSF() (and fixed a bug...)
removed superfluous tests for the case when gridDataFormats package cannot be found: gridDataFormats is a dependency so we can always assume it is present

PR Checklist

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

IAlibay · 2020-02-18T08:38:19Z

So not directly related to issue #2502, but whilst things are being deprecated. As far as I am aware, density_from_PDB had historically been untested, would it be worth maybe introducing a plan to remove it unless someone is willing to write tests for it?

IAlibay

Just a couple of suggested changes, otherwise looks good.

package/MDAnalysis/analysis/density.py

testsuite/MDAnalysisTests/analysis/test_density.py

codecov · 2020-02-18T12:49:02Z

Codecov Report

Merging #2537 into develop will increase coverage by 0.07%.
The diff coverage is 98.00%.

@@             Coverage Diff             @@
##           develop    #2537      +/-   ##
===========================================
+ Coverage    90.71%   90.79%   +0.07%     
===========================================
  Files          174      174              
  Lines        23552    23597      +45     
  Branches      3073     3075       +2     
===========================================
+ Hits         21366    21425      +59     
+ Misses        1565     1548      -17     
- Partials       621      624       +3

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/base.py	`95.65% <ø> (-4.35%)`	⬇️
package/MDAnalysis/analysis/density.py	`89.09% <98.00%> (+23.64%)`	⬆️
package/MDAnalysis/analysis/hole2/hole.py	`73.40% <0.00%> (-8.78%)`	⬇️
package/MDAnalysis/coordinates/TRZ.py	`82.33% <0.00%> (-1.51%)`	⬇️
package/MDAnalysis/analysis/lineardensity.py	`81.81% <0.00%> (-1.30%)`	⬇️
package/MDAnalysis/analysis/gnm.py	`95.27% <0.00%> (-0.79%)`	⬇️
package/MDAnalysis/lib/formats/libdcd.pyx	`78.31% <0.00%> (-0.65%)`	⬇️
package/MDAnalysis/lib/util.py	`87.66% <0.00%> (-0.59%)`	⬇️
package/MDAnalysis/core/topologyobjects.py	`97.51% <0.00%> (-0.36%)`	⬇️
... and 7 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 a78307f...1c2e943. Read the comment docs.

lilyminium

This is nice. Mostly comments on docs

package/MDAnalysis/analysis/density.py

orbeckst · 2020-02-18T16:37:44Z

@IAlibay and @lilyminium many thanks for the excellent review, I'll work on it.

I'll throw out metadata and some of the other historic crud – this is the best opportunity to clean up a bit.

orbeckst · 2020-02-19T08:11:32Z

Cleaned up history. Please review. Thanks.

IAlibay

Just a couple of extra tests if possible.

testsuite/MDAnalysisTests/analysis/test_density.py

package/MDAnalysis/analysis/density.py

(need two lines before ..versionchanged)

- deprecate in 1.0.0 - remove in 2.0.0 - adjusted tests that test for warnings: only test for UserWarning and match the message with regex in pytest.warns() instead manually looking through the WarningReport object: the problem is that the deprecation warning changes the number of entries

These tests are not necessary because gridDataFormats is a dependency.

@nawtrey

- close #2502 - DensityAnalysis is based on @nawtrey's pmda.density.DensityAnalysis but replace _reduce with accumulation in _single_frame - added tests (based on the Test_density_from_universe): both give the same answer as they pass the same tests; test specific exceptions in DensityAnalysis - update docs (also write ångström consistently throughout) - update CHANGELOG

When density_from_universe() is gone we will not need the function factory either.

- density_from_PDB is terrible code (I wrote it) and not used: deprecated and to be removed for 2.0.0 - added test so that it is covered - fixed bug that was uncovered by test (us ag.tempfactors) - added test for Bfactors2RMSF - test for deprecation warnings - organized in classes - shortened test names for density_from_universe()

orbeckst · 2020-02-20T00:12:39Z

Docs should pass, too – took me a while to figure out that I cannot just @deprecate a class... using it on __init__ works although it leaves a crappy message.

IAlibay

A possible test failure and two questions.

testsuite/MDAnalysisTests/analysis/test_density.py

package/MDAnalysis/analysis/density.py

orbeckst · 2020-02-26T22:19:47Z

I am not ignoring all your excellent reviews, I just don't have time to work on it at the moment! If anyone else wants to finish the PR, please do! Otherwise I'll get to it... when I get to it.

orbeckst · 2020-03-11T23:55:54Z

@IAlibay I am assigning you as the official chaperone for this PR.

Perhaps @wvandertoorn wants to finish this PR? That would be awesome.

wvdtoorn · 2020-03-12T07:05:04Z

@IAlibay @orbeckst Definitely, I'm on it!

- [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] 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

Partially fixes #2502 (completes PR #2537) ## PR contents: This PR by @wvandertoorn finalises the work done as part of #2537 Key commits: - Fixes docstring (adds .. deprecated information). - Adds back :class:`TestGridImport`. - Reduces precision matching of `test_density_from_PDB`. - CHANGELOG and AUTHORS updated accordingly.

IAlibay · 2020-03-15T14:28:48Z

@orbeckst just cleaned up the merge conflict to get travis et al. to run (I assume that's why appveyor/pr failed straight away...).

IAlibay

Some changes I didn't catch in @wvandertoorn's PR. I'd blame it on the lack of Travis checks on #2616, but I probably should have picked these up 😱

package/MDAnalysis/analysis/density.py

testsuite/MDAnalysisTests/analysis/test_density.py

wvdtoorn · 2020-03-15T21:21:37Z

@IAlibay I'm sorry to have missed it myself, wasn't familiar with sphynx before. I'll take care of the indentation issues right away, but the import failure isn't trivial too me, that might take a bit longer. Would you prefer the indentation and import fixes in separate PR's or in one?

IAlibay · 2020-03-15T21:36:02Z

@IAlibay I'm sorry to have missed it myself, wasn't familiar with sphynx before. I'll take care of the indentation issues right away, but the import failure isn't trivial too me, that might take a bit longer. Would you prefer the indentation and import fixes in separate PR's or in one?

@wvandertoorn if you look at the file difference, it looks like all you have to do is replace the unittest.mock import with: from mock import Mock, patch. My understanding here is that MDAnalysis uses the mock package: https://github.com/testing-cabal/mock which backports unittest.mock to pre-3.6 python.

wvdtoorn · 2020-03-15T22:02:46Z

@wvandertoorn if you look at the file difference, it looks like all you have to do is replace the unittest.mock import with: from mock import Mock, patch. My understanding here is that MDAnalysis uses the mock package: https://github.com/testing-cabal/mock which backports unittest.mock to pre-3.6 python.

Take it back, trivial indeed. Wasn't aware of testing-cabal's mock package, neat.

IAlibay

Ok, now that Travis is green, I've run out of things to review.

@lilyminium any more comments?

@orbeckst are you ok with the PR as it is now?

lilyminium

This looks great, I really like all the examples! Super tiny nitpicks.

I did think that ..versionchanged:: etc entries came in reverse-chronological order though, i.e. from most recent to least?

lilyminium · 2020-03-17T02:37:35Z

package/MDAnalysis/analysis/density.py


+      After the analysis (see the :meth:`~DensityAnalysis.run` method), the resulting density is
+      stored in the :attr:`density` attribut as a :class:`Density` instance.


attribut[e]

lilyminium · 2020-03-17T03:36:40Z

package/MDAnalysis/analysis/density.py

+        """
+        """


Extra empty string

IAlibay · 2020-03-17T09:48:10Z

This looks great, I really like all the examples! Super tiny nitpicks.

I did think that ..versionchanged:: etc entries came in reverse-chronological order though, i.e. from most recent to least?

@lilyminium I'm not sure we've been super consistent in this unfortunately.

I've had a look through and for the most part there seems to be quite a few cases of .. versionchanged being in the order of oldest to newest. We probably need to agree on a convention and stick to it though.

Example cases:
topologyobjects, rms, lib/distances, PQRParser, MOL2Parser, analysis/init, core/groups, core/universe, topology/base, (kinda gave up looking after that).

@wvandertoorn would you like to raise a separate PR to address @lilyminium's comments?

wvdtoorn · 2020-03-17T10:31:28Z

@IAlibay Yes, will do

IAlibay

lgtm, just waiting on final comments by @orbeckst

orbeckst

Thank you @wvandertoorn (and @IAlibay and @lilyminium ), all lgtm!

orbeckst · 2020-03-18T23:09:18Z

@IAlibay , if you could do the honors?

orbeckst · 2020-03-18T23:09:53Z

p.s.: Merge so that @wvandertoorn 's commits survive.

IAlibay · 2020-03-19T04:11:27Z

p.s.: Merge so that @wvandertoorn 's commits survive.

@orbeckst Being wary of these new merge powers, it's probably best I ask for clarification here. What's the preferred option for MDAnalysis when trying to conserve commits on merge, merge rebase or a merge commit?

orbeckst · 2020-03-19T16:37:45Z

If it's a simple fix from a single contributor then we typically squash&merge. We especially want to remove little micro-updates and reversals that make no sense when you read the commit history.

If it's a bigger thing with

multiple non-trivial contributions
multiple self-contained commits

then we typically just merge.

If committers are experienced with git we sometimes ask them to rewrite their history, especially if they care about individual commits.

If someone who isn't the original author of the commits rebased then we typically also merge because in the past we found that on a double-rebase, the original committer is lost. This might also be a case when history rewriting and commit-condensing is necessary.

update: ASV (the automatic benchmarks) is configured so that they only run on merge commits, therefore if anything is affecting actual code then you should not rebase & merge because this will not trigger ASV.

IAlibay · 2020-03-19T17:54:56Z

Thanks for the explanation @orbeckst that helped clear things up 😄

Congrats on your first commits @wvandertoorn 🎉

orbeckst · 2020-03-19T18:03:31Z

Thanks @wvandertoorn for finalizing this features – makes me very happy that this managed to get into 1.0, thanks to your push.

orbeckst requested review from richardjgowers, IAlibay and lilyminium February 18, 2020 08:09

orbeckst mentioned this pull request Feb 18, 2020

[WIP] Analysis MDAnalysis/UserGuide#38

Closed

48 tasks

IAlibay requested changes Feb 18, 2020

View reviewed changes

lilyminium requested changes Feb 18, 2020

View reviewed changes

orbeckst force-pushed the densityanalysis-2502 branch from cd5a1d1 to f5b337b Compare February 19, 2020 08:11

IAlibay requested changes Feb 19, 2020

View reviewed changes

testsuite/MDAnalysisTests/analysis/test_density.py Show resolved Hide resolved

package/MDAnalysis/analysis/density.py Show resolved Hide resolved

orbeckst force-pushed the densityanalysis-2502 branch from f5b337b to d376f70 Compare February 19, 2020 23:42

orbeckst added 7 commits February 19, 2020 17:08

fixed reST in AnalysisBase

bed3523

(need two lines before ..versionchanged)

removed tests for missing gridDataFormats

72654fa

These tests are not necessary because gridDataFormats is a dependency.

deprecate density.notwithin_coordinates_factory

548c43f

When density_from_universe() is gone we will not need the function factory either.

Bfactor2RMSF:simple test

9764455

orbeckst force-pushed the densityanalysis-2502 branch from d376f70 to 1154532 Compare February 20, 2020 00:11

IAlibay requested changes Feb 20, 2020

View reviewed changes

testsuite/MDAnalysisTests/analysis/test_density.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/analysis/test_density.py Show resolved Hide resolved

package/MDAnalysis/analysis/density.py Show resolved Hide resolved

IAlibay mentioned this pull request Mar 10, 2020

Tests should make MDAnalysis raise exceptions #597

Open

orbeckst assigned IAlibay Mar 11, 2020

wvdtoorn and others added 2 commits March 15, 2020 14:15

Merge branch 'develop' into densityanalysis-2502

422370f

IAlibay requested changes Mar 15, 2020

View reviewed changes

package/MDAnalysis/analysis/density.py Outdated Show resolved Hide resolved

package/MDAnalysis/analysis/density.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/analysis/test_density.py Outdated Show resolved Hide resolved

wvdtoorn mentioned this pull request Mar 15, 2020

Indentation and import fix #2629

Merged

4 tasks

Indentation and import fix (#2629)

4cfdec6

IAlibay reviewed Mar 16, 2020

View reviewed changes

lilyminium approved these changes Mar 17, 2020

View reviewed changes

wvdtoorn mentioned this pull request Mar 17, 2020

Wrap up loose ends from PR review #2633

Merged

4 tasks

Wrap up loose ends from PR review (#2633)

1bfb9a8

IAlibay approved these changes Mar 18, 2020

View reviewed changes

orbeckst commented Mar 18, 2020

View reviewed changes

Merge branch 'develop' into densityanalysis-2502

1c2e943

IAlibay merged commit e0883c8 into develop Mar 19, 2020

orbeckst deleted the densityanalysis-2502 branch March 19, 2020 18:03

fiona-naughton added enhancement Component-Analysis labels Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new DensityAnalysis #2537

new DensityAnalysis #2537

orbeckst commented Feb 18, 2020 •

edited

Loading

IAlibay commented Feb 18, 2020 •

edited

Loading

IAlibay left a comment

codecov bot commented Feb 18, 2020 •

edited

Loading

lilyminium left a comment

orbeckst commented Feb 18, 2020

orbeckst commented Feb 19, 2020

IAlibay left a comment

orbeckst commented Feb 20, 2020

IAlibay left a comment

orbeckst commented Feb 26, 2020

orbeckst commented Mar 11, 2020

wvdtoorn commented Mar 12, 2020

IAlibay commented Mar 15, 2020

IAlibay left a comment

wvdtoorn commented Mar 15, 2020

IAlibay commented Mar 15, 2020

wvdtoorn commented Mar 15, 2020

IAlibay left a comment

lilyminium left a comment

lilyminium Mar 17, 2020

lilyminium Mar 17, 2020

IAlibay commented Mar 17, 2020

wvdtoorn commented Mar 17, 2020

IAlibay left a comment

orbeckst left a comment

orbeckst commented Mar 18, 2020

orbeckst commented Mar 18, 2020

IAlibay commented Mar 19, 2020

orbeckst commented Mar 19, 2020 •

edited

Loading

IAlibay commented Mar 19, 2020

orbeckst commented Mar 19, 2020


		After the analysis (see the :meth:`~DensityAnalysis.run` method), the resulting density is
		stored in the :attr:`density` attribut as a :class:`Density` instance.

new DensityAnalysis #2537

new DensityAnalysis #2537

Conversation

orbeckst commented Feb 18, 2020 • edited Loading

PR Checklist

IAlibay commented Feb 18, 2020 • edited Loading

IAlibay left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2020 • edited Loading

Codecov Report

lilyminium left a comment

Choose a reason for hiding this comment

orbeckst commented Feb 18, 2020

orbeckst commented Feb 19, 2020

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst commented Feb 20, 2020

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst commented Feb 26, 2020

orbeckst commented Mar 11, 2020

wvdtoorn commented Mar 12, 2020

IAlibay commented Mar 15, 2020

IAlibay left a comment

Choose a reason for hiding this comment

wvdtoorn commented Mar 15, 2020

IAlibay commented Mar 15, 2020

wvdtoorn commented Mar 15, 2020

IAlibay left a comment

Choose a reason for hiding this comment

lilyminium left a comment

Choose a reason for hiding this comment

lilyminium Mar 17, 2020

Choose a reason for hiding this comment

lilyminium Mar 17, 2020

Choose a reason for hiding this comment

IAlibay commented Mar 17, 2020

wvdtoorn commented Mar 17, 2020

IAlibay left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Mar 18, 2020

orbeckst commented Mar 18, 2020

IAlibay commented Mar 19, 2020

orbeckst commented Mar 19, 2020 • edited Loading

IAlibay commented Mar 19, 2020

orbeckst commented Mar 19, 2020

orbeckst commented Feb 18, 2020 •

edited

Loading

IAlibay commented Feb 18, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

orbeckst commented Mar 19, 2020 •

edited

Loading