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/standardize query args #395

Merged
merged 19 commits into from Aug 22, 2019
Merged

Feature/standardize query args #395

merged 19 commits into from Aug 22, 2019

Conversation

vyasr
Copy link
Collaborator

@vyasr vyasr commented Aug 21, 2019

Description

This PR cleans up some of the C++ code for the QueryArgs class, introducing more constants into the namespace and improving code consistency. Query argument names have been standardized further, and updated throughout freud. In particular, most references to rmax, nn, num_neigh, etc have been removed in favor of the standard names r_max and num_neighbors. On the Python side, more of the QueryArgs logic has been standardized by introducing a PairCompute subclass of Compute that encapsulates the standard code that goes at the beginning of every compute or accumulate call. In the process, I've also defined a standard method for classes to expose a set of default query arguments.

Motivation and Context

This PR is necessary to standardize the usage of query arguments (especially default query arguments) throughout freud. It's also consistent with #179 and related changes for improving the API.

How Has This Been Tested?

No new tests have been added yet, they will need to be added once the code is closer to a final state. The main tests we will need to add are ensuring that default query arguments work as expected, and that invalid QueryArgs objects are properly rejected.

Screenshots (if appropriate):

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 Changelog.

@vyasr vyasr added enhancement New feature or request locality labels Aug 21, 2019
@vyasr vyasr added this to the v2.0 milestone Aug 21, 2019
@vyasr vyasr requested a review from bdice August 21, 2019 20:20
@vyasr vyasr self-assigned this Aug 21, 2019
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.

Very helpful! @vyasr you said this PR would be hard to review, but I think it went smoothly. 😄
Some suggestions for edits below:

cpp/locality/AABBQuery.h Outdated Show resolved Hide resolved
freud/common.pyx Show resolved Hide resolved
freud/common.pyx Outdated Show resolved Hide resolved
freud/common.pyx Outdated Show resolved Hide resolved
freud/density.pyx Show resolved Hide resolved
freud/environment.pyx Outdated Show resolved Hide resolved
freud/locality.pxd Outdated Show resolved Hide resolved
freud/locality.pxd Outdated Show resolved Hide resolved
freud/order.pyx Outdated Show resolved Hide resolved
tests/test_density_ComplexCF.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #395 into next will decrease coverage by 0.34%.
The diff coverage is 83.49%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next     #395      +/-   ##
==========================================
- Coverage   93.14%   92.79%   -0.35%     
==========================================
  Files          15       15              
  Lines        3034     3027       -7     
  Branches       19       19              
==========================================
- Hits         2826     2809      -17     
- Misses        200      210      +10     
  Partials        8        8
Impacted Files Coverage Δ
freud/order.pyx 97.78% <100%> (ø) ⬆️
freud/locality.pxd 92% <100%> (ø) ⬆️
freud/density.pyx 97.95% <100%> (-0.08%) ⬇️
freud/locality.pyx 94.16% <66.66%> (-0.67%) ⬇️
freud/common.pyx 89.69% <70.83%> (-6.2%) ⬇️
freud/environment.pyx 88.93% <92.59%> (-0.08%) ⬇️

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 beaca81...e15dab0. Read the comment docs.

@vyasr vyasr marked this pull request as ready for review August 22, 2019 18:39
@vyasr vyasr requested a review from a team as a code owner August 22, 2019 18:39
@vyasr vyasr requested a review from bdice August 22, 2019 18:39
@vyasr vyasr mentioned this pull request Aug 22, 2019
9 tasks
@vyasr
Copy link
Collaborator Author

vyasr commented Aug 22, 2019

I will draft relevant documentation and add to #348 soon.

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.

Great work! I approve this.

@vyasr vyasr merged commit f644ccf into next Aug 22, 2019
@vyasr vyasr mentioned this pull request Aug 23, 2019
10 tasks
@vyasr vyasr deleted the feature/standardize_query_args branch September 9, 2019 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request locality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants