improve promotion and equality checking #52

mikmoore · 2021-12-20T23:42:15Z

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

Just a few minor style comments.

src/Quaternion.jl

dkarrasch · 2021-12-30T11:32:13Z

This should close #40, IIUC.

better styling and formatting Co-authored-by: Daniel Karrasch <[email protected]>

mikmoore · 2021-12-31T03:44:55Z

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 · 2022-02-19T20:53:40Z

Codecov Report

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

@@            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%)`	⬆️
src/Octonion.jl	`28.04% <100.00%> (+18.04%)`	⬆️
src/Quaternion.jl	`87.50% <100.00%> (+2.44%)`	⬆️

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.

sethaxen

Thanks for the PR! Can you add tests for == for Octonion and DualQuaternion as well? Also, most of the conversions modified here are not yet covered by tests, and they should be tested as well.

src/DualQuaternion.jl

src/Octonion.jl

Co-authored-by: Seth Axen <[email protected]>

sethaxen

Sorry, accidentally approved before the missing tests were added.

test/test_Quaternion.jl

Co-authored-by: Seth Axen <[email protected]>

…orm field. but they should probably be removed altogether.

mikmoore · 2022-02-20T04:01:21Z

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

Aliasing the shorthand functions would be a breaking change. While the public API of this package is not documented, they are exported functions and should be considered part of the API. As for their use quat is to Quaternion as complex is to Complex. The former is a method the user can overload to quaternion-ify anything, while the latter is a constructor that should return a Quaternion (and probably should not be overloaded). You could open an issue to discuss removing them, but that really should not be in this PR.

src/DualQuaternion.jl

sethaxen · 2022-02-20T11:36:19Z

src/DualQuaternion.jl

@@ -6,29 +6,29 @@ struct DualQuaternion{T<:Real} <: Number
  norm::Bool
 end

-DualQuaternion(q0::Quaternion, qe::Quaternion, n::Bool = false) =
+DualQuaternion(q0::Quaternion, qe::Quaternion, n::Bool = q0.norm) =


This does not seem to be consistent with how norm is defined for DualQuaternion. norm returns a Dual number, and the DualQuaternion is normalized when the real part of the norm is 1 and the infinitesimal part is 0.

I think we only want to set norm=true when its value can be determined from either the types being provided or from the value of other norm fields, which I don't think we can do here.

I have zero knowledge of dual quaternions, hence my mistake. It appears this has veered wildly beyond the scope of both this PR and my understanding and I'll revert my change.

Off-topic discussion: wouldn't your statement imply that n::Bool = q0.norm & iszero(qe) would be the proper initialization? More importantly, doesn't any requirement on the infinitesimal part imply that normalize(::DualQuaternion) and friends are incorrect in setting norm=true without examining the infinitesimal? According to https://en.wikipedia.org/wiki/Dual_quaternion#Norm, however, it appears to be slightly less restrictive than what you have claimed. It says that a dual quaternion A+eB is unit when (AA*,AB*+BA*)=(1,0).

TBH, I don't have much experience with dual quaternions either, so I can't really answer the questions without digging into the code and the theory a bit more. My point was just that the change would create inconsistencies in the code base.

sethaxen · 2022-02-20T11:49:17Z

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

Co-authored-by: Seth Axen <[email protected]>

…wrong .norm field. but they should probably be removed altogether." This reverts commit 3809886.

mikmoore · 2022-02-20T13:55:57Z

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.

into JuliaGeometry-master

sethaxen

If you browse through the code, CodeCov has annotated a few lines changed here that are not covered by the tests. Can you check that they are? Also, can you add tests that the shorthand cases you mentioned where norm was set differently than by the constructor are also tested?

test/test_Quaternion.jl

Co-authored-by: Seth Axen <[email protected]>

test/test_Quaternion.jl

Co-authored-by: Seth Axen <[email protected]>

sethaxen

LGTM, thanks!

improve promotion and equality checking

da6c867

dkarrasch reviewed Dec 30, 2021

View reviewed changes

src/Quaternion.jl Outdated Show resolved Hide resolved

src/Quaternion.jl Outdated Show resolved Hide resolved

dkarrasch mentioned this pull request Dec 30, 2021

Fix conversion of Complex to Quaternion/Octonion #40

Closed

mikmoore and others added 3 commits December 30, 2021 17:33

Apply suggestions from code review

fc1d79a

better styling and formatting Co-authored-by: Daniel Karrasch <[email protected]>

change && to & in ==

2043d2a

extend promotion and equality improvements to Octonion, DualQuaternion

a90f73f

sethaxen closed this Feb 19, 2022

sethaxen reopened this Feb 19, 2022

sethaxen requested changes Feb 19, 2022

View reviewed changes

src/DualQuaternion.jl Show resolved Hide resolved

src/Octonion.jl Outdated Show resolved Hide resolved

sethaxen added bug enhancement labels Feb 19, 2022

Update src/Octonion.jl

092ccf9

Co-authored-by: Seth Axen <[email protected]>

sethaxen approved these changes Feb 20, 2022

View reviewed changes

sethaxen requested changes Feb 20, 2022

View reviewed changes

add promotion and equality tests for DualQuaternion and Octonion

c9e5035

sethaxen mentioned this pull request Feb 20, 2022

Extend all analytic functions in base for quaternions #56

Merged

sethaxen requested changes Feb 20, 2022

View reviewed changes

test/test_Quaternion.jl Outdated Show resolved Hide resolved

mikmoore and others added 4 commits February 19, 2022 19:23

Update test/test_Quaternion.jl

902a852

Co-authored-by: Seth Axen <[email protected]>

reorganize testing and add convert() tests

359fb0b

fix troublesome shorthand constructors that gave potentially-wrong .n…

3809886

…orm field. but they should probably be removed altogether.

fix broken DualQuaternion interactions with DualNumbers.Dual

d21747b

sethaxen reviewed Feb 20, 2022

View reviewed changes

mikmoore and others added 3 commits February 20, 2022 06:34

Update src/DualQuaternion.jl

627f820

Co-authored-by: Seth Axen <[email protected]>

revert mistake to norm field

1823d86

Revert "fix troublesome shorthand constructors that gave potentially-…

4083684

…wrong .norm field. but they should probably be removed altogether." This reverts commit 3809886.

quat(), dualquat(), and octo() don't set their own defaults for norm

1f60958

mikmoore added 4 commits February 20, 2022 07:07

make Quaternion(1) === Quaternion{T}(1)

c9bf65e

fix norm fields of DualQuaternion(::Dual)

1a80a42

Merge branch 'master' of https://github.com/JuliaGeometry/Quaternions.jl

a259170

into JuliaGeometry-master

Merge branch 'JuliaGeometry-master' into promotion

8d3eedc

sethaxen reviewed Feb 20, 2022

View reviewed changes

test/test_Quaternion.jl Outdated Show resolved Hide resolved

mikmoore and others added 5 commits February 20, 2022 07:46

fix test that was broken because it was misplaced

f83d75c

Update test/test_Quaternion.jl

8c534e2

Co-authored-by: Seth Axen <[email protected]>

increase test coverage

6787ccc

a few more tests

94a65a4

Merge branch 'master' into promotion

693eaa0

sethaxen reviewed Feb 20, 2022

View reviewed changes

test/test_Quaternion.jl Show resolved Hide resolved

Update test/test_Quaternion.jl

c333db7

Co-authored-by: Seth Axen <[email protected]>

sethaxen approved these changes Feb 20, 2022

View reviewed changes

sethaxen merged commit cbb1a07 into JuliaGeometry:master Feb 20, 2022

sethaxen mentioned this pull request Feb 20, 2022

Should Complex be convertible to Quaternion? #62

Closed

This was referenced Feb 21, 2022

one(Quaternion{Float64}) should have normalized-flag true #59

Closed

Not full support Quaternion{BigFloat} #25

Closed

update exp tests for norm flag #66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve promotion and equality checking #52

improve promotion and equality checking #52

mikmoore commented Dec 20, 2021

dkarrasch left a comment

dkarrasch commented Dec 30, 2021

mikmoore commented Dec 31, 2021

codecov bot commented Feb 19, 2022 •

edited

Loading

sethaxen left a comment

sethaxen left a comment

mikmoore commented Feb 20, 2022

sethaxen left a comment

sethaxen Feb 20, 2022

sethaxen Feb 20, 2022

mikmoore Feb 20, 2022

sethaxen Feb 20, 2022

sethaxen commented Feb 20, 2022

mikmoore commented Feb 20, 2022

sethaxen left a comment

sethaxen left a comment

improve promotion and equality checking #52

improve promotion and equality checking #52

Conversation

mikmoore commented Dec 20, 2021

dkarrasch left a comment

Choose a reason for hiding this comment

dkarrasch commented Dec 30, 2021

mikmoore commented Dec 31, 2021

codecov bot commented Feb 19, 2022 • edited Loading

Codecov Report

sethaxen left a comment

Choose a reason for hiding this comment

sethaxen left a comment

Choose a reason for hiding this comment

mikmoore commented Feb 20, 2022

sethaxen left a comment

Choose a reason for hiding this comment

sethaxen Feb 20, 2022

Choose a reason for hiding this comment

sethaxen Feb 20, 2022

Choose a reason for hiding this comment

mikmoore Feb 20, 2022

Choose a reason for hiding this comment

sethaxen Feb 20, 2022

Choose a reason for hiding this comment

sethaxen commented Feb 20, 2022

mikmoore commented Feb 20, 2022

sethaxen left a comment

Choose a reason for hiding this comment

sethaxen left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2022 •

edited

Loading