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

Feature/rdf normalization #529

Merged
merged 5 commits into from
Oct 20, 2019
Merged

Feature/rdf normalization #529

merged 5 commits into from
Oct 20, 2019

Conversation

vyasr
Copy link
Collaborator

@vyasr vyasr commented Oct 17, 2019

Description

Enable normalization of RDF to 1 in the large R limit. Also fix a bug in Jupyter plotting code.

Motivation and Context

Resolves: #396. Also fixes an unrelated bug.

How Has This Been Tested?

Screenshots

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds or improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • I have updated the documentation (if relevant).
  • I have added tests that cover my changes (if relevant).
  • All new and existing tests passed.
  • I have updated the credits.
  • I have updated the Changelog.

@vyasr vyasr added enhancement New feature or request density labels Oct 17, 2019
@vyasr vyasr added this to the v2.0 final milestone Oct 17, 2019
@vyasr vyasr requested review from joaander and bdice October 17, 2019 22:19
@vyasr vyasr requested a review from a team as a code owner October 17, 2019 22:19
@vyasr vyasr self-assigned this Oct 17, 2019
@vyasr
Copy link
Collaborator Author

vyasr commented Oct 17, 2019

@joaander see #396 for the relevant discussion, I wanted to get your input before finalizing this.

Copy link
Collaborator

@mphoward mphoward 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 fixing! This looks like it should work to me.

@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #529 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
+ Coverage   90.33%   90.39%   +0.05%     
==========================================
  Files          16       16              
  Lines        2049     2050       +1     
  Branches       27       27              
==========================================
+ Hits         1851     1853       +2     
+ Misses        185      184       -1     
  Partials       13       13
Impacted Files Coverage Δ
freud/density.pyx 96.85% <100%> (+0.65%) ⬆️

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 c9ea2f7...a7c25f4. Read the comment docs.

Copy link
Member

@joaander joaander 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!

Levine, Stone, and Kohlmeyer have a clean way of expressing g(r) normalization with arbitrary particle selections if you would like to generalize this or use their notation in the documentation.

@vyasr
Copy link
Collaborator Author

vyasr commented Oct 18, 2019

Thanks for the paper! I think we could implement that generally here, it would just require maintaining a counter inside the loop in compute to determine the total number of pairs that we computed for and then normalizing for that case. However, it seems like a pretty esoteric use-case, so I don't think it's a priority and I'm happy to punt on it for now. @bdice thoughts? If you agree with me, then go ahead and merge this PR when you're happy and we can reconsider that at a future date.

@mphoward
Copy link
Collaborator

mphoward commented Oct 18, 2019

Josh makes a good point. It actually probably isn't too hard to treat this normalization exactly here because freud does not have such a strong sense of "selections" / "groups" or "tags" as other programs. The number of pairs in the normalization is N*(N-1) iff query_points is neighbors AND exclude_ii is True. Otherwise, the normalization is for N*N pairs, some of which may be excluded. The normalize flag would activate this pair-counting logic (and could include a link to that paper); otherwise, it would fall back on the old logic and always use the N*N pairs.

Copy link
Member

@bdice bdice 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
density enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect RDF normalization
4 participants