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

Fix conversion of Complex to Quaternion/Octonion #40

Closed
wants to merge 1 commit into from
Closed

Fix conversion of Complex to Quaternion/Octonion #40

wants to merge 1 commit into from

Conversation

dkarrasch
Copy link
Contributor

Not sure if this addresses #29, but at least it allows the Base promotion to work. This method was previously not tested, and probably not working.

@goretkin
Copy link

goretkin commented Aug 15, 2020

Can you speak to the semantics of this operation? If I got it, this operation is consistent with the definition at https://github.com/JuliaGeometry/Quaternions.jl/blob/master/src/Quaternion.jl#L15 to convert Complex to Quaternion.

One possible answer to the semantics question is that this multiplication corresponds to rotation in a specific plane. Have there been cases where you've wanted to do this?

@dkarrasch
Copy link
Contributor Author

The changed methods currently don't work at all, because there is no Quaternions.imag(::Complex). Remember that imag is not imported from Base, and I think this is done on purpose. I noticed this while writing tests for LinearMaps.jl, but didn't need it in the end, so we can as well close this.

@dkarrasch dkarrasch changed the title Allow *(::Complex, ::Quaternion) Fix conversion of Complex to Quaternion/Octonion Dec 30, 2021
@dkarrasch
Copy link
Contributor Author

Gentle bump. I still needed it in LinearMaps.jl tests, in which I needed a non-commutative number type multiplied with complex numbers. I renamed the PR to reflect that this actually fixes broken conversion methods.

@dkarrasch
Copy link
Contributor Author

I guess this is superseded by #52, but I'll leave it open in case there is something controversial about #52.

@sethaxen
Copy link
Collaborator

Closing this now because #52 was merged.

@sethaxen sethaxen closed this Feb 20, 2022
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 this pull request may close these issues.

3 participants