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

Remove zero(::UnitQuaternion) #159

Merged
merged 11 commits into from
Oct 17, 2021

Conversation

hyrodium
Copy link
Collaborator

This PR fixes #157.

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2021

Codecov Report

Merging #159 (8aa6e26) into master (38b208b) will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   85.40%   85.57%   +0.17%     
==========================================
  Files          12       12              
  Lines        1261     1269       +8     
==========================================
+ Hits         1077     1086       +9     
+ Misses        184      183       -1     
Impacted Files Coverage Δ
src/unitquaternion.jl 96.17% <ø> (+0.52%) ⬆️
src/core_types.jl 89.83% <100.00%> (+0.83%) ⬆️

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 38b208b...8aa6e26. Read the comment docs.

@hyrodium
Copy link
Collaborator Author

hyrodium commented May 19, 2021

I've also added ArgumentError to avoid zero(<:Rotation).

Before this PR

julia> zero(RotMatrix3{Float64})
3×3 RotMatrix3{Float64} with indices SOneTo(3)×SOneTo(3):
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

After this PR

julia> zero(RotMatrix3{Float64})
ERROR: ArgumentError: There is no additive identity for Rotation. Consider using the one(RotMatrix3{Float64}) method instead.
Stacktrace:
 [1] zero(T::Type{RotMatrix3{Float64}})
   @ Rotations ~/.julia/dev/Rotations/src/core_types.jl:14
 [2] top-level scope
   @ REPL[2]:1

@hyrodium hyrodium changed the title Remove zero(::UnitQuaternion) Remove zero(::UnitQuaternion) May 19, 2021
@@ -11,6 +11,8 @@ Base.@pure StaticArrays.Size(::Type{Rotation{N,T}}) where {N,T} = Size(N,N)
Base.@pure StaticArrays.Size(::Type{R}) where {R<:Rotation} = Size(supertype(R))
Base.adjoint(r::Rotation) = inv(r)
Base.transpose(r::Rotation{N,T}) where {N,T<:Real} = inv(r)
Base.zero(T::Type{<:Rotation}) = throw(ArgumentError("There is no additive identity for Rotation. Consider using the one($T) method instead."))
Base.zero(r::Rotation) = throw(ArgumentError("There is no additive identity for Rotation. Consider using the one($(typeof(r))($r)) method instead."))

Choose a reason for hiding this comment

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

See comment at #157 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's an updated error message:

"$T is a subtype of AbstractMatrix, but there is no additive identity for Rotation. To get a non-rotation instance, consider using the one($T) method instead."

Is there any problem with this? I'm not native English speaker, so there may be some mistakes. :)

@andyferris
Copy link
Contributor

andyferris commented May 20, 2021

Thanks @hyrodium.

I'm a bit concerned about the argument error, though. Every rotation is meant to be a fully-functioning AbstractMatrix. Generic code might call zero(x), in which case we can just return an SMatrix. Just like rotation + rotation gives an SMatrix (you can think of zero() providing the identity for the + operation, which is defined but not closed) so should zero.

Currently we have:

julia> r = one(RotMatrix3{Float64})
3×3 RotMatrix3{Float64} with indices SOneTo(3)×SOneTo(3):
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> r + r
3×3 StaticArrays.SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 2.0  0.0  0.0
 0.0  2.0  0.0
 0.0  0.0  2.0

julia> zero(r)
3×3 RotMatrix3{Float64} with indices SOneTo(3)×SOneTo(3):
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

EDIT - we should fix up this last one

We are extending the AbstractArray interface here - as a general rule, we can add functionality but not subtract functionality.

@hyrodium
Copy link
Collaborator Author

At first, I thought zero(T) should be a instance of T, but now, I agree with that zero(::Rotation) returns SMatrix{3,3}(0,0,0,0,0,0,0,0,0).

I'll fix it!

@hyrodium
Copy link
Collaborator Author

I've updated the zero funciton.

julia> zero(MRP)
3×3 StaticArrays.SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> zero(RotMatrix{3})
3×3 StaticArrays.SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> zero(RotMatrix{2})
2×2 StaticArrays.SMatrix{2, 2, Float64, 4} with indices SOneTo(2)×SOneTo(2):
 0.0  0.0
 0.0  0.0

julia> zero(Angle2d)
2×2 StaticArrays.SMatrix{2, 2, Float64, 4} with indices SOneTo(2)×SOneTo(2):
 0.0  0.0
 0.0  0.0

But, there sitll be some issues around zeros funciton.

julia> zeros(MRP,2)
2-element Vector{MRP}:
 [1.0 0.0 0.0; 0.0 1.0 0.0; 0.0 0.0 1.0]
 [1.0 0.0 0.0; 0.0 1.0 0.0; 0.0 0.0 1.0]

julia> zeros(RotMatrix{3},2)
2-element Vector{RotMatrix{3, T, L} where {T, L}}:
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]

julia> zeros(RotMatrix{2},2)
2-element Vector{RotMatrix{2, T, L} where {T, L}}:
 [0.0 0.0; 0.0 0.0]
 [0.0 0.0; 0.0 0.0]

julia> zeros(Angle2d,2)
2-element Vector{Angle2d}:
Error showing value of type Vector{Angle2d}:
ERROR: MethodError: no method matching cos(::NTuple{4, Float64})

I'll fix them after tomorrow.

@andyferris
Copy link
Contributor

Awesome.

At first, I thought zero(T) should be a instance of T

There was an old discussion at julialang/julia about this once... here's a recent one quoting the docs: JuliaLang/julia#40796 (comment)

Basically we want things like isequal(x, x + zero(x)) and isequal(x, x * one(x)), but not always that zero(x) isa typeof(x) or one(x) isa typeof(x).

@hyrodium
Copy link
Collaborator Author

I've updated zeros method:

julia> zeros(MRP,2)
2-element Vector{StaticArrays.SMatrix{3, 3, Float64, 9}}:
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]

julia> zeros(RotMatrix{3},2)
2-element Vector{StaticArrays.SMatrix{3, 3, Float64, 9}}:
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]
 [0.0 0.0 0.0; 0.0 0.0 0.0; 0.0 0.0 0.0]

julia> zeros(RotMatrix{2},2)
2-element Vector{StaticArrays.SMatrix{2, 2, Float64, 4}}:
 [0.0 0.0; 0.0 0.0]
 [0.0 0.0; 0.0 0.0]

julia> zeros(Angle2d,2)
2-element Vector{StaticArrays.SMatrix{2, 2, Float64, 4}}:
 [0.0 0.0; 0.0 0.0]
 [0.0 0.0; 0.0 0.0]

Usually, zeros(T,n) returns a instance of Vector{T}. (e.g. zeros(Real, 3))
But, zeros(MRP,2) can't be Vector{MRP} because zero(MRP) <: MRP is false.

@hyrodium
Copy link
Collaborator Author

There was an old discussion at julialang/julia about this once... here's a recent one quoting the docs: JuliaLang/julia#40796 (comment)

Basically we want things like isequal(x, x + zero(x)) and isequal(x, x * one(x)), but not always that zero(x) isa typeof(x) or one(x) isa typeof(x).

Thanks for the information!

@hyrodium
Copy link
Collaborator Author

bump :)

@andyferris andyferris merged commit f024b07 into JuliaGeometry:master Oct 17, 2021
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.

zero(UnitQuaternion) doesn't make sense (for me)
4 participants