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

Should Complex be convertible to Quaternion? #62

Closed
hyrodium opened this issue Feb 19, 2022 · 3 comments · Fixed by #113
Closed

Should Complex be convertible to Quaternion? #62

hyrodium opened this issue Feb 19, 2022 · 3 comments · Fixed by #113
Labels

Comments

@hyrodium
Copy link
Collaborator

We already have constructor and convert method from Complex to Quaternion.

julia> Quaternion(Complex(1,2))
Quaternion{Int64}(1, 2, 0, 0, false)

julia> convert(Quaternion, Complex(1,2))
Quaternion{Int64}(1, 2, 0, 0, false)

However, we still missing the following methods + and *.

julia> Quaternion(1,2,3,4) + Complex(1,2)
ERROR: MethodError: no method matching imag(::Complex{Int64})
You may have intended to import Base.imag
Closest candidates are:
  imag(::Quaternion) at ~/.julia/dev/Quaternions/src/Quaternion.jl:44
  imag(::Octonion) at ~/.julia/dev/Quaternions/src/Octonion.jl:48
Stacktrace:
 [1] convert(#unused#::Type{Quaternion{Int64}}, z::Complex{Int64})
   @ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:21
 [2] _promote
   @ ./promotion.jl:327 [inlined]
 [3] promote
   @ ./promotion.jl:350 [inlined]
 [4] +(x::Quaternion{Int64}, y::Complex{Int64})
   @ Base ./promotion.jl:379
 [5] top-level scope
   @ REPL[23]:1

julia> Quaternion(1,2,3,4) * Complex(1,2)
ERROR: MethodError: no method matching imag(::Complex{Int64})
You may have intended to import Base.imag
Closest candidates are:
  imag(::Quaternion) at ~/.julia/dev/Quaternions/src/Quaternion.jl:44
  imag(::Octonion) at ~/.julia/dev/Quaternions/src/Octonion.jl:48
Stacktrace:
 [1] convert(#unused#::Type{Quaternion{Int64}}, z::Complex{Int64})
   @ Quaternions ~/.julia/dev/Quaternions/src/Quaternion.jl:21
 [2] _promote
   @ ./promotion.jl:327 [inlined]
 [3] promote
   @ ./promotion.jl:350 [inlined]
 [4] *(x::Quaternion{Int64}, y::Complex{Int64})
   @ Base ./promotion.jl:380
 [5] top-level scope
   @ REPL[24]:1

I think we have the following choices to solve this problem.

  • Remove the constructor and convert method
    • There is no natural embedding ℂ ⊆ ℍ because it depends on the choice of the basis.
    • This will make a breaking change.
  • Add type promotions
    • This will not make a breaking change.

I don't know the practical benefit of converting from Complex to Quaternion, so I prefer the first.

Related comment: #29 (comment)

@sethaxen
Copy link
Collaborator

I agree there is not natural embedding of in , and it would be better if we did not have this conversions. Removing these would be breaking.

But this is orthogonal to the error at the beginning, which is because Quaternions has its own imag function that is different from Base.imag, and it's calling the wrong with in the convert method:

convert(::Type{Quaternion{T}}, z::Complex) where {T} =
Quaternion(convert(T, real(z)), convert(T, imag(z)), convert(T, 0), convert(T, 0))

Base.imag needs to be explicitly called here. This is repaired in #56, but a separate PR to fix the bug wouldn't be a bad thing.

@dkarrasch
Copy link
Contributor

Yes, there has been a quick fix for the conversion issue around in #40 (ignoring the general embedding issue).

@sethaxen
Copy link
Collaborator

The bug was fixed in #52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants