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 norm field in Quaternion #243

Merged
merged 5 commits into from
Jan 3, 2023

Conversation

hyrodium
Copy link
Collaborator

The norm field was deprecated in JuliaGeometry/Quaternions.jl#110, and will be removed in JuliaGeometry/Quaternions.jl#108. This PR removes the norm field in Quaternion.

Copy link

@mikmoore mikmoore left a comment

Choose a reason for hiding this comment

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

It looks like the same functionality is repeated here multiple times. This was true even before this PR, but I would take this opportunity to consolidate the implementations by forwarding to a single method. Thought should also be given to how to handle inputs that cannot result in well-defined rotations (zero and nonfinite inputs).

@inline function QuatRotation{T}(q::Quaternion) where T
if q.norm
new{T}(q)
@inline function QuatRotation{T}(q::Quaternion, normalize::Bool = true) where T

Choose a reason for hiding this comment

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

This appears to be missing a check for iszero(q) (on either the input or output - undecided), which should probably result in an error. I'd be willing to permit !isfinite(q) to pass (the result will presumably be a NaN quaternion) but someone may prefer that errors as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current behavior of constructors other than the Quaternion is also not checked for input correctness, so I would like to separate the feature into another PR.

julia> AngleAxis(0.0, 0.0, 0.0, 0.0)
3×3 AngleAxis{Float64} with indices SOneTo(3)×SOneTo(3)(0.0, NaN, NaN, NaN):
 NaN  NaN  NaN
 NaN  NaN  NaN
 NaN  NaN  NaN

julia> QuatRotation(0.0, 0.0, 0.0, 0.0)
3×3 QuatRotation{Float64} with indices SOneTo(3)×SOneTo(3)(QuaternionF64(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> RotMatrix{3}([1 1 1;2 2 2;3 3 3])
3×3 RotMatrix3{Int64} with indices SOneTo(3)×SOneTo(3):
 1  1  1
 2  2  2
 3  3  3

src/unitquaternion.jl Show resolved Hide resolved
src/unitquaternion.jl Outdated Show resolved Hide resolved
src/unitquaternion.jl Outdated Show resolved Hide resolved
@hyrodium
Copy link
Collaborator Author

hyrodium commented Jan 2, 2023

The CI tests are canceled due to their time limit. I think this is because of the deprecated warnings from Quaternions.jl, and will be fixed by merging #245. The test succeeds on my local machine, so I'll merge this PR in a few days.

@hyrodium hyrodium merged commit 894f524 into JuliaGeometry:master Jan 3, 2023
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.

2 participants