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

RDF Normalization Mode Argument #1013

Merged
merged 17 commits into from Oct 12, 2022
Merged

RDF Normalization Mode Argument #1013

merged 17 commits into from Oct 12, 2022

Conversation

tommy-waltmann
Copy link
Collaborator

@tommy-waltmann tommy-waltmann commented Sep 27, 2022

Description

This PR changes the normalize argument to freud.density.RDF to be more descriptive, and clarifies what the default value is.

Motivation and Context

There was a lot of discussion about how the RDF should be normalized, what the options should be, and how the API should look in the issues linked below.

Resolves: #635
Related to #396

How Has This Been Tested?

I have added a test for both of the valid arguments, and a test case for the invalid arguments.

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)
  • Documentation improvement (updates to user guides, docstrings, or developer docs)

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.

@tommy-waltmann tommy-waltmann requested a review from a team as a code owner September 27, 2022 14:41
@tommy-waltmann tommy-waltmann requested review from DomFijan and removed request for a team September 27, 2022 14:41
@DomFijan
Copy link
Contributor

I can take a look at this on Friday.

@tommy-waltmann tommy-waltmann added this to the v3.0.0 milestone Sep 27, 2022
Copy link
Contributor

@DomFijan DomFijan left a comment

Choose a reason for hiding this comment

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

This looks good. I just two questions related to docstrings.

freud/density.pyx Outdated Show resolved Hide resolved
freud/density.pyx Outdated Show resolved Hide resolved
@vyasr
Copy link
Collaborator

vyasr commented Oct 3, 2022

I've updated the PR description to say that this is related to #396 but not that it resolves it. There is additional work to be done to resolve that issue.

freud/density.pyx Outdated Show resolved Hide resolved
@tommy-waltmann
Copy link
Collaborator Author

I've updated the PR description to say that this is related to #396 but not that it resolves it. There is additional work to be done to resolve that issue.

Good point, I'm not currently planning on implementing the new solution on this PR, so it wouldn't resolve that issue.

Copy link
Contributor

@DomFijan DomFijan left a comment

Choose a reason for hiding this comment

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

I think there is a typo in one of the changes to docstrings (please double check that). And I have some improvements suggested. I can't see anything else wrong.

freud/density.pyx Outdated Show resolved Hide resolved
freud/density.pyx Outdated Show resolved Hide resolved
freud/density.pyx Outdated Show resolved Hide resolved
tommy-waltmann and others added 2 commits October 6, 2022 14:41
Co-authored-by: Domagoj Fijan <50439291+DomFijan@users.noreply.github.com>
Co-authored-by: Domagoj Fijan <50439291+DomFijan@users.noreply.github.com>
Copy link
Collaborator

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

It would be nice to actually validate the long-distance behaviors of the RDF for a small system of particles as well, but getting that test to behave nicely statistically will probably be hard since the whole point is to test a small system where statistics are bad, so if you have a hard time writing a robust test it's fine to skip it.

cpp/density/RDF.h Show resolved Hide resolved
doc/source/conf.py Show resolved Hide resolved
doc/source/gettingstarted/migration.rst Outdated Show resolved Hide resolved
doc/source/gettingstarted/migration.rst Outdated Show resolved Hide resolved
freud/density.pyx Outdated Show resolved Hide resolved
freud/density.pyx Outdated Show resolved Hide resolved
tommy-waltmann and others added 2 commits October 10, 2022 10:42
Co-authored-by: Vyas Ramasubramani <vramasub@umich.edu>
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.

None yet

3 participants