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

Fix/pmftxyz eq ors normalization #619

Merged
merged 7 commits into from Sep 11, 2020
Merged

Conversation

vyasr
Copy link
Collaborator

@vyasr vyasr commented May 18, 2020

Description

Store and use the last number of equivalent orientations to normalize the PMFT. This PR should be merged after #615 and only following discussion regarding whether this solution is appropriate on #616.

Motivation and Context

Resolves: #616.

How Has This Been Tested?

Updated existing test to include checking the behavior when multiple equivalent quaternions are specified.

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 requested a review from a team as a code owner May 18, 2020 14:06
@vyasr vyasr requested a review from bdice May 18, 2020 14:06
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #619 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   92.78%   92.87%   +0.09%     
==========================================
  Files          16       16              
  Lines        2189     2189              
  Branches       32       32              
==========================================
+ Hits         2031     2033       +2     
+ Misses        146      144       -2     
  Partials       12       12              
Impacted Files Coverage Δ
freud/pmft.pyx 98.92% <0.00%> (+1.07%) ⬆️

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 3f44951...195ce82. Read the comment docs.

@bdice bdice added pmft bug Something isn't working labels May 18, 2020
@bdice bdice added this to the v2.3 milestone May 18, 2020
@bdice bdice changed the base branch from master to fix/pmftxyt_orientation May 19, 2020 13:50
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.

I'd like to have some discussion on #619 before we accept this solution.

@vyasr
Copy link
Collaborator Author

vyasr commented May 21, 2020

I'd like to have some discussion on #619 before we accept this solution.

I assume you mean #616 as mentioned in the PR.

Base automatically changed from fix/pmftxyt_orientation to master May 25, 2020 19:43
@vyasr vyasr changed the base branch from master to doc/cubatic_doc June 3, 2020 16:27
@vyasr vyasr changed the base branch from doc/cubatic_doc to master June 3, 2020 16:27
@bdice bdice modified the milestones: v2.3, v2.4 Aug 1, 2020
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.

LGTM, unless there's further discussion on #616.

@vyasr vyasr merged commit ea161bd into master Sep 11, 2020
@vyasr vyasr deleted the fix/pmftxyz_eq_ors_normalization branch September 11, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pmft
Projects
None yet
2 participants