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

Native quaternion implementation #166

Merged
merged 17 commits into from
Nov 14, 2024
Merged

Native quaternion implementation #166

merged 17 commits into from
Nov 14, 2024

Conversation

arahlin
Copy link
Member

@arahlin arahlin commented Oct 14, 2024

This PR removes the dependency on boost::math::quaternion in favor of a minimal native implementation, which includes simple arithmetic and comparison operators, and conj, abs, norm and pow function implementations. The Quat object is serializeable and provides a numpy buffer interface.

Closes #143.

@arahlin arahlin requested a review from nwhitehorn October 14, 2024 23:39
@arahlin arahlin self-assigned this Oct 14, 2024
@arahlin arahlin force-pushed the g3quat branch 4 times, most recently from 511431b to 5c1907f Compare October 15, 2024 04:54
This PR removes the dependency on boost::math::quaternion in favor of a minimal
native implementation, which includes simple arithmetic and comparison
operators, and conj, abs, norm and pow function implementations.

The class also provides versor() and is_versor() methods for creating and
testing for unit quaternions, used in many pointing operations in the maps
package.

The G3Quat object is a true serializeable G3FrameObject with a numpy buffer
interface.  The derived G3VectorQuat and G3MapQuat classes maintain backward
compatibility for deserializing data stored on disk.
@nwhitehorn
Copy link
Member

I had one large-scale question. Making G3Quat a frame object instead of a primitive type makes a few things more complicated:

  • It will slow down conversions between numpy arrays and quaternions and G3VectorQuat
  • It will likely make a G3VectorQuat occupy a significant amount more memory
  • Because of the less-dense format, it may impede some compiler vectorization operations

Have you looked at the change in storage size or other performance metrics?

@arahlin
Copy link
Member Author

arahlin commented Oct 18, 2024

I haven't yet. That said, it would be straightforward to walk back the frameobject change and leave the serialization and vectorization as is.

@arahlin
Copy link
Member Author

arahlin commented Oct 18, 2024

I think you're right that the G3VectorQuat object ends up 50% larger, just based on the change in the length of strides[0].

Use a separate G3Quat derived class for serialization into a frame,
analogously to the simple G3Data quantities.
@@ -1536,7 +1536,7 @@ PYBINDINGS("maps") {
"a disc of the given radius at the given sky coordinates.")

.def("query_disc",
(std::vector<size_t> (G3SkyMap::*)(quat, double) const)
(std::vector<size_t> (G3SkyMap::*)(const Quat &, double) const)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason to do this? If quat is the same type as Quat basically, I'm not sure this is any more efficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, this made some more sense when Quat was derived from G3FrameObject.

Copy link
Member Author

@arahlin arahlin Oct 19, 2024

Choose a reason for hiding this comment

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

It looks like it's O(10ns) of overhead for the value argument vs const reference for a single operation. Pretty small, but might add up?

@nwhitehorn
Copy link
Member

This basically looks good to me. It looks like it would reduce the diff by quite a lot if you did s/Quat/quat. Is there a reason to change the class name?

@arahlin
Copy link
Member Author

arahlin commented Nov 11, 2024

This basically looks good to me. It looks like it would reduce the diff by quite a lot if you did s/Quat/quat. Is there a reason to change the class name?

Just to differentiate this class from the boost one, and to make it clear that it's a class object with Ptr, ConstPtr, Vector derived classes.

core/src/G3Quat.cxx Outdated Show resolved Hide resolved
@arahlin arahlin merged commit 136e6e8 into master Nov 14, 2024
2 checks passed
@arahlin arahlin deleted the g3quat branch November 20, 2024 17:13
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.

Incorrect behavior of numpy.abs() on G3VectorQuat objects
2 participants