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%> (ø)`

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

hyrodium commented 1 year ago

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

Replacing abs2 causes failures with tests added by https://github.com/JuliaGeometry/Quaternions.jl/pull/82#discussion_r841934935 . 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: https://github.com/JuliaGeometry/Quaternions.jl/pull/119#discussion_r1045101872)

Do you have any thoughts on this?

sethaxen commented 1 year ago

Fix realdot definition (x-ref:

I think this makes the most sense.

mikmoore commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

hyrodium commented 1 year ago

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

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

JuliaGeometry / Quaternions.jl

Add support for `RealDot` #119

Codecov Report