Skip to content

Fix InterRDF_s normalization problem when given density = True#121

Merged
orbeckst merged 7 commits intoMDAnalysis:masterfrom
VOD555:fix_density
Jul 8, 2020
Merged

Fix InterRDF_s normalization problem when given density = True#121
orbeckst merged 7 commits intoMDAnalysis:masterfrom
VOD555:fix_density

Conversation

@VOD555
Copy link
Collaborator

@VOD555 VOD555 commented Jun 27, 2020

Fixes #120

Changes made in this Pull Request:

  • Change the N=nA * nB to be 1, as this is a single-atom to single-atom RDF calculaton.

PR Checklist

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

@VOD555 VOD555 changed the title fix number of atoms Fix InterRDF_s normalization problem when given density = True Jun 27, 2020
@VOD555 VOD555 requested review from Balasubra and orbeckst and removed request for Balasubra June 27, 2020 22:14
@VOD555
Copy link
Collaborator Author

VOD555 commented Jun 27, 2020

Tests for density function failed as described in #122 .

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

lgtm – but please also raise an issue for serial MDA, especially as we want to do a quick bug-fix release 1.0.1 MDAnalysis/mdanalysis#2768



@pytest.fixture(scope='module')
def rdf_ref(u):
Copy link
Member

Choose a reason for hiding this comment

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

Is the reference the normal RDF calculation with a single atom in each group?

rdf = []

for i, (nA, nB) in enumerate(self.ag_shape):
for i in range(len(self.ag_shape)):
Copy link
Member

Choose a reason for hiding this comment

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

Is this also broken in MDAnalysis.analysis.rdf.InterRDF_s?

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

  • update CHANGELOG
  • merge master

@pep8speaks
Copy link

pep8speaks commented Jul 7, 2020

Hello @VOD555! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-08 16:39:06 UTC

@orbeckst
Copy link
Member

orbeckst commented Jul 7, 2020

I merged master.

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   97.93%   97.93%           
=======================================
  Files          12       12           
  Lines         775      775           
  Branches       91       91           
=======================================
  Hits          759      759           
  Misses         10       10           
  Partials        6        6           
Impacted Files Coverage Δ
pmda/rdf.py 100.00% <100.00%> (ø)

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 e98ebcc...44a6b3f. Read the comment docs.

@orbeckst
Copy link
Member

orbeckst commented Jul 8, 2020

Update the CHANGELOG and fix the PEP8 issues and then this should be good to go. Or do you have any other concerns, @VOD555?

@orbeckst orbeckst merged commit 99104d7 into MDAnalysis:master Jul 8, 2020
@VOD555 VOD555 deleted the fix_density branch July 12, 2020 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong results from InterRDF_s when given 'density=True'

3 participants