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

Drop inefficient functions for rotation? #86

Closed
hyrodium opened this issue Apr 29, 2022 · 3 comments · Fixed by #111
Closed

Drop inefficient functions for rotation? #86

hyrodium opened this issue Apr 29, 2022 · 3 comments · Fixed by #111
Labels

Comments

@hyrodium
Copy link
Collaborator

The following functions return Array which is less efficient than StaticArrays.StaticArray.

  • rotationmatrix
  • rotationmatrix_normalized
  • axis

These can be replaced with efficient functions in Rotations.jl package. Adding warning messages to replace these functions with Rotations.jl package, and removing them in the next breaking release would be better, I guess.

Both Quaternions.jl and Rotations.jl packages are maintained under JuliaGeometry organization, so I think this change will not be problematic.

julia> using Quaternions, Rotations

julia> q = nquatrand()
QuaternionF64(0.09045921662676236, 0.9292679982955513, -0.0983890390919598, 0.34438018882858595, true)

julia> axis(q)
3-element Vector{Float64}:
  0.9330935318067537
 -0.09879407893716233
  0.34579790466015914

julia> rotation_axis(QuatRotation(q))
3-element StaticArrays.SVector{3, Float64} with indices SOneTo(3):
  0.9330935318067537
 -0.09879407893716233
  0.3457979046601592

julia> rotationmatrix(q)
3×3 Matrix{Float64}:
  0.743444  -0.245164   0.622243
 -0.120555  -0.964273  -0.235888
  0.657843   0.100355  -0.746439

julia> QuatRotation(q)
3×3 QuatRotation{Float64} with indices SOneTo(3)×SOneTo(3)(QuaternionF64(0.0904592, 0.929268, -0.098389, 0.34438, true)):
  0.743444  -0.245164   0.622243
 -0.120555  -0.964273  -0.235888
  0.657843   0.100355  -0.746439
@hyrodium
Copy link
Collaborator Author

hyrodium commented Sep 20, 2022

angle is not that inefficient, but I think Base.angle(::Quaternion) should be removed for consistency.

julia> using Quaternions

julia> angle(1+im)
0.7853981633974483

julia> angle(Quaternion(1+im))  # This issue is related to https://github.com/JuliaGeometry/Quaternions.jl/issues/62
1.5707963267948966

julia> angle(-1)
3.141592653589793

julia> angle(Quaternion(-1,0,0,0))  # This issue is not related to complex-quaternion compatibility
6.283185307179586

julia> -1 == Quaternion(-1,0,0,0)
true

x-ref: #62

@hyrodium
Copy link
Collaborator Author

I wonder if someone doesn't need that high performance with Rotations.jl but wants to avoid heavy load time with StaticArrays.jl.

I'm planning to remove the functions after adding deprecated messages, but if you want that functionality, feel free to comment on this issue. We can consider reverting them later.

@mikmoore
Copy link
Contributor

I think removing these would be fine. I'm okay with Quaternions.jl being a primarily mathematical package and all the rotational utilities being delegated to Rotations.jl. Any rotational stuff we do in Quaternions.jl would end up replicated or re-exported in Rotations.jl anyway.

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.

2 participants