-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add Transforms Testcases and fix some stuff #696
Conversation
Hello @kevindlewis23! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-08-08 16:33:38 UTC |
@@ -309,10 +317,7 @@ def xy_to_gvec( | |||
array_like, optional | |||
if output_ref is True | |||
""" | |||
# TODO: in the C library beam vector and eta vector are expected. However | |||
# we receive rmat_b. Please check this! |
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.
This function agrees with the numpy version, which takes rmat_b directly and does not do this decomposition.
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.
Just a few small requested changes. After those are made, this will be good to go!
I tested these changes using a variety of different workflows, and they appeared to work fine.
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.
Looks good to me, but there are still merge conflicts.
I checked these testcases against Oscar's numpy implementation and the old C version as well, here are the results:
angles_to_gvec
andangles_to_dvec
: All agree!gvec_to_xy
: C versions agree on values, although the return NaN’s at different times. The sometimes agree with the numpy version, although numpy’s version ignorestvec_s
while the C versions ignorevmat_inv
andbmat
(discussion with Donald Boyce)xy_to_gvec
: C versions agree, numpy version agrees after some transpositions (couldn’t figure out what transpositions are needed and I had to transpose at a few points in the numpy implementation rather than just on the inputs, but presumably there’s a way to do it. I just didn’t want to try every possible combination of transpositionsmake_beam_rmat
: C versions agree, numpy gives the inverse resultmake_rmat_of_expmap
: All agree!make_sample_rmat
: All agree!quat_distance
: C versions agree, numpy version gives a different result about half the time (not depending on the number of symmetry quaternions, it’s about half for any number).xf.py
has a very similar implementation to numpy’s with a few small changes, and it agrees with C versions. So, I assume the C versions are correctrotate_vecs_about_axis
andunit_vector
: Tested directly and both C and numpy versions are correctvalidate_angle_ranges
: Only one C implementation. Seems to always agree on ccw inputs, often disagrees when the inputs are not ccw. I wrote my own super simple implementation as well and it agrees with the C version 17/18 times on ccw and 21/22 times on cw.The numpy implementation is definitely wrong on cw inputs, I tested with ([1], [pi], [0], False) and it didn’t work, while the C implementation did.
Also, both return true on ([0], [0], [1]), but only the C version returns true on ([1], [0], [1]), and actually accepts some error on purpose. Even ([1+1e-8], [0], [1]) returns true.
I ran some direct tests and the C implementation passed everything, unlike the numpy version and the version in
xf.py
, which sometimes doesn't work when there are multiple ranges.