JuliaGeometry / Quaternions.jl

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

Add support for `reim` #88

Closed hyrodium closed 2 years ago

hyrodium commented 2 years ago

The method Base.imag(q::Quaternion) should not be defined because the output type must be changed, but Base.reim(c::Complex) is a Tuple, so Base.reim(q::Quaternion) can be defined with return type Tuple.

codecov[bot] commented 2 years ago

Codecov Report

Merging #88 (0088456) into master (4cf7f2d) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
+ Coverage   97.21%   97.22%   +0.01%     
==========================================
  Files           3        3              
  Lines         395      397       +2     
==========================================
+ Hits          384      386       +2     
  Misses         11       11              
Impacted Files Coverage Δ
src/Octonion.jl 96.66% <100.00%> (+0.03%) :arrow_up:
src/Quaternion.jl 98.51% <100.00%> (+<0.01%) :arrow_up:

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

hyrodium commented 2 years ago

Oh, I just noticed reim(a::Real) is a NTuple{2, Real}.

julia> reim(3)
(3, 0)

Should we close this PR, or rename Base.reim(q::Quaternion) with something like Quaternions.reim_quat(q::Quaternion)?

sethaxen commented 2 years ago

Oh, I just noticed reim(a::Real) is a NTuple{2, Real}.

julia> reim(3)
(3, 0)

Should we close this PR, or rename Base.reim(q::Quaternion) with something like Quaternions.reim_quat(q::Quaternion)?

I agree, I would prefer a different function name than reim, but let me ask, what's the benefit of including any such function at all? Are there places where it substantially simplifies/reduces the code?

hyrodium commented 2 years ago

but let me ask, what's the benefit of including any such function at all? Are there places where it substantially simplifies/reduces the code?

Slightly improvement, but we can reduce these two lines to one line. https://github.com/JuliaGeometry/Rotations.jl/blob/524f61d48396f3f604e90d422917efc247b9f58a/src/unitquaternion.jl#L126-L127

When I made this PR, I thought adding a method to reim would be better than imag_part because imag_part is not a function in Base. I now agree with avoiding adding method to reim, and It seems okay with real and imag_part instead of reim.