Add support for RealDot

[hyrodium]

This PR adds RealDot.realdot(::Quaternion, ::Quaternion) as suggested in #58 (comment).

[codecov]

Codecov Report

Merging #119 (1e3e17c) into main (268cee0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #119   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          135       136    +1     
=========================================
+ Hits           135       136    +1     

Impacted Files	Coverage Δ
src/Quaternion.jl	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sethaxen]

LGTM, just some minor suggestions.

There are 2 places in the code where we might consider using realdot:

Base.abs2(q::Quaternion) = RealDot.realdot(q, q)
# in slerp
coshalftheta = RealDot.realdot(qa, qb)

Merge with a patch version bump when you feel it's ready.

[hyrodium]

There are 2 places in the code where we might consider using realdot:

Replacing abs2 causes failures with tests added by #82 (comment) . We have the following choices to fix this problem.

• Keep the original abs2 definition
• Replace @test abs2(q) == q'*q with @test abs2(q) ≈ q'*q
• Fix realdot definition (x-ref: Add support for RealDot #119 (comment))

Do you have any thoughts on this?

[sethaxen]

Fix realdot definition (x-ref:

I think this makes the most sense.

[mikmoore]

I am going to suggest that realdot be made to match the calculation of real(p*q) EDIT: real(p'*q) -- i.e., copy the first line of the * definition. I already spent significant effort in #82 making sure that abs2(q) and real(q*q') are exactly equal and as fast as I could manage. Anything but exact equality can cause headaches for users. Any speed difference will be negligible outside of microbenchmarks.

EDIT: Also, make sure that code_native((p,q)->real(p*q),NTuple{2,QuaternionF64}) doesn't succeed in dropping the imaginary parts of the calculation. If it does, then I would define both realdot and abs2 in terms of * so that we don't need to worry about keeping the implementations synchronized any more. The compiler failed to do this a year ago, but it's worth trying again while we're here (having removed the old .norm field might have helped).

[sethaxen]

I am going to suggest that realdot be made to match the calculation of real(p*q) -- i.e., copy the first line of the * definition.

It should be real(conj(p) * q), but otherwise, yes, I agree.

Also, make sure that code_native((p,q)->real(p*q),NTuple{2,QuaternionF64}) doesn't succeed in dropping the imaginary parts of the calculation. If it does, then I would define both realdot and abs2 in terms of * so that we don't need to worry about keeping the implementations synchronized any more. The compiler failed to do this a year ago, but it's worth trying again while we're here (having removed the old .norm field might have helped).

Even if the compiler manages to not calculate the imaginary part, it's still worth having these overloads, because common code transformations, such as automatic differentiation still happen at an earlier step in compilation process; the resulting code is more complex, and the compiler might not be able to eliminate the unused branch. Plus we support older Julia versions as well, and the downsides to implementing realdot and abs2 as we do are only needing to maintain 2 more LOCs, which is negligible.

[hyrodium]

Thank you for the comments! This PR is ready for final review.

[sethaxen]

I think we also said that we could use realdot for abs2?

[hyrodium]

I think we also said that we could use realdot for abs2?

Oh, sorry, I was forgetting that. I have updated that:+1:

[sethaxen]

LGTM, thanks! Just please increment the patch version and make a patch release.