pep8speaks
commented
1 month ago 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! :beers:
I checked these testcases against Oscar's numpy implementation and the old C version as well, here are the results:
angles_to_gvecandangles_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_swhile the C versions ignorevmat_invandbmat(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.pyhas 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_axisandunit_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 inxf.py, which sometimes doesn't work when there are multiple ranges.