JuliaGeometry / Quaternions.jl

A Julia implementation of quaternions
https://juliageometry.github.io/Quaternions.jl
MIT License
115 stars 37 forks source link

improve promotion and equality checking #52

Closed mikmoore closed 2 years ago

mikmoore commented 2 years ago

This PR is primarily motivated by an issue where Quaternion(1) == 1 returned false. There were two issues at play here.

First, convert(Quaternion,::Real) and convert(Quaternion,::Complex) were never setting the .norm field of the resulting Quaternion. This meant that Quaternion(1.0).norm == true while convert(Quaternion,1.0).norm == false.

Second, there was no special-cased ==(::Quaternion,::Quaternion) method. This meant that equality required the .norm fields to match.

This PR reroutes convert calls through the normal constructors to help .norm be set more consistently. It also adds a == method for Quaternions so that the .norm field is ignored in equality checks. Lastly, it adds some tests along these lines.

dkarrasch commented 2 years ago

This should close #40, IIUC.

mikmoore commented 2 years ago

I noticed that & is probably better than && for short chains of simple comparisons (no branches), so I changed that slightly although it can certainly be reverted. I also extended this PR to include the matching changes to Octonion and DualQuaternion, which I had neglected before.

codecov[bot] commented 2 years ago

Codecov Report

Merging #52 (c333db7) into master (be0fc14) will increase coverage by 9.84%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage   45.58%   55.43%   +9.84%     
==========================================
  Files           3        3              
  Lines         351      359       +8     
==========================================
+ Hits          160      199      +39     
+ Misses        191      160      -31     
Impacted Files Coverage Δ
src/DualQuaternion.jl 21.78% <100.00%> (+17.65%) :arrow_up:
src/Octonion.jl 28.04% <100.00%> (+18.04%) :arrow_up:
src/Quaternion.jl 87.50% <100.00%> (+2.44%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update be0fc14...c333db7. Read the comment docs.

mikmoore commented 2 years ago

I noted that the internal(?) shorthand functions quat, dualquat, and octo had the same issue of defaulting the norm field to false, in addition to being totally redundant. I redefined them to be constants equal to their corresponding type, which should fix that. In the grander scheme I think they really had ought to be removed, but that's beyond the scope of this PR.

I added explicit conversion tests. They revealed that DualQuaternion was broken when given DualNumbers.Dual because the field names were wrong. I believe I fixed that as well, but somebody should check because dual numbers aren't really my thing.

sethaxen commented 2 years ago

Also, can you rebase on master and resolve merge conflicts?

mikmoore commented 2 years ago

As for their use quat is to Quaternion as complex is to Complex.

I see. In that case, I changed them to not default norm=false but instead to only forward a norm flag if provided one. This will let them inherit default norm behavior from the relevant constructors. Without this change, for example, Quaternion(1) and quat(1) resulted in different norm flags.