-
Notifications
You must be signed in to change notification settings - Fork 49
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
cpp/order/HexaticTranslational.cc
Outdated
m_psi_array[i] /= std::complex<float>(m_k); | ||
if (total_weight == 0.) | ||
{ | ||
m_psi_array[i] /= std::complex<float>(total_weight); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
@klywang any updates on this? |
for more information, see https://pre-commit.ci
…b/freud into feature/zero-neighbors
We checked the behavior of Hexatic, Translational, SolidLiquid, and LocalDescriptors handle cases with zero neighbors.
For particles without neighbors:
nan
forwl=True
andwl=False
nan
particle_harmonics
returnsnan
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
Checklist: