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

Added tests and documentation for cases zero neighbor cases. #775

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

klywang
Copy link
Contributor

@klywang klywang commented May 25, 2021

We checked the behavior of Hexatic, Translational, SolidLiquid, and LocalDescriptors handle cases with zero neighbors.

For particles without neighbors:

  • Steinhardt: Returns nan for wl=True and wl=False
  • Hexatic: Returns nan
  • Translational: Returns 0
  • SolidLiquid: particle_harmonics returns nan
  • LocalDescriptors: num_sphs returns 0, sph=[]

Description

Added tests for edge cases and updated docs where nan is expected.

TODO: Had some issue building the docs. Most of the notes did not show up.

Motivation and Context

Resolves: #678

How Has This Been Tested?

Added unit tests.

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)
  • 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.

@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #775 (3c152ae) into master (b960cab) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
+ Coverage   55.38%   55.48%   +0.09%     
==========================================
  Files          16       16              
  Lines        2580     2581       +1     
  Branches       38       38              
==========================================
+ Hits         1429     1432       +3     
+ Misses       1135     1133       -2     
  Partials       16       16              
Impacted Files Coverage Δ
freud/order.pyx 45.91% <100.00%> (+0.86%) ⬆️

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 b960cab...3c152ae. Read the comment docs.

m_psi_array[i] /= std::complex<float>(m_k);
if (total_weight == 0.)
{
m_psi_array[i] /= std::complex<float>(total_weight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Force this to be nan by dividing by zero. Is there a better way to do this? I saw std::nanf but I wasn't sure what to pick as the argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't reviewed this PR thoroughly yet, but I'm not sure this is generating the output you want. std::complex<float>(1)/std::complex<float>(0) will return (inf, nan), not just nan (here's an example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So tried to set m_psy_array equal to just nan like this:

m_psi_array[i] = std::complex<float>(std::nanf)

and then like this

m_psi_array[i] = std::complex<float>(std::nanf)

All tests pass on my laptop, but fail on CI, so I reverted back to a divide by zero. I double checked the values from the no neighbor test for the Translational order parameter, and in python, it registers as nan and not inf.

@vyasr
Copy link
Collaborator

vyasr commented Nov 27, 2021

@klywang any updates on this?

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.

Check how order parameters treat particles with zero neighbors
3 participants