-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add more methods to quat
#132
Conversation
|
||
quat(p, v1, v2, v3) = Quaternion(p, v1, v2, v3) | ||
quat(x) = Quaternion(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this removal would be breaking. e.g. this would block users from constructing quaternions with SymbolicUtils.Num
or Unitful.Quantity
. Not that I'm opposed to it, but we need to be certain we're fine with that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a breaking change.
No problem with Symbolics.Num
because it's a subtype of Real
.
julia> using Symbolics, Quaternions
julia> a = Symbolics.Num(3)
3
julia> quat(a,a,a,a)
Quaternion{Num}(3, 3, 3, 3)
julia> quat(a)
Quaternion{Num}(3, 0, 0, 0)
julia> Symbolics.Num <: Real
true
Unitful.Quantitiy
is already not compatible with Quaternions.jl.
Before this PR
julia> using Unitful, Quaternions
julia> d = 3u"°"
3°
julia> quat(d)
ERROR: MethodError: no method matching Quaternion(::Quantity{Int64, NoDims, Unitful.FreeUnits{(°,), NoDims, nothing}})
Closest candidates are:
(::Type{T})(::T) where T<:Number
@ Core boot.jl:792
(::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number}
@ Base char.jl:50
(::Type{T})(::Base.TwicePrecision) where T<:Number
@ Base twiceprecision.jl:266
...
After this PR
julia> using Unitful, Quaternions
julia> d = 3u"°"
3°
julia> quat(d)
ERROR: MethodError: no method matching quat(::Quantity{Int64, NoDims, Unitful.FreeUnits{(°,), NoDims, nothing}})
Closest candidates are:
quat(::Quaternion)
@ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:52
quat(::Real)
@ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:53
quat(::Real, ::Real, ::Real, ::Real)
@ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:54
...
Co-authored-by: Seth Axen <seth@sethaxen.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 162 173 +11
=========================================
+ Hits 162 173 +11 ☔ View full report in Codecov by Sentry. |
493e665
to
8f4c091
Compare
Abstract
This PR adds more methods to
quat
.Before this PR
After this PR
This behavior is consistent with
Base.complex
.How to find the methods to add
I have confirmed the methods with the
methods
function.Note that this PR does not contain methods with
SparseArrays
because there were some performance degressions.The above definition
Quaternions.quat(S::SparseMatrixCSC)
is from:https://github.com/JuliaSparse/SparseArrays.jl/blob/bd2bda8f45ed71374223068b0a0daa2b169071c9/src/sparsematrix.jl#L984