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

Use Quaternions.Quaternion instead of ReferenceFrameRotations.Quaternion? #25

Closed
hyrodium opened this issue Jan 17, 2023 · 5 comments
Closed

Comments

@hyrodium
Copy link
Contributor

Hi! I'm a maintainer of Quaternions.jl, and I wonder if we can replace ReferenceFrameRotations.Quaternion with Quaternions.Quaternion. Before 2022, Quaternions.jl was not well-maintained, but the package is now ready to be used in other packages. See my post on Julia discourse for more information.

If this change is acceptable, I will be glad to open a PR.
I think the hardest part of the migration is avoiding type piracy. (e.g. Quaternion(u::UniformScaling{T}) should not be defined.)

@ronisbr
Copy link
Member

ronisbr commented Jan 17, 2023

Hi @hyrodium !

In a typical scenario, I would say why not! However, ReferenceFrameRotations.jl is used here at INPE for satellite simulators and analyses.

Since we do not have time right now to update all the tools we have, the migration must respect some points:

  1. Everything must be the same, with no API or breaking changes. This includes, for example, the naming convention for the quaternion elements and the output print to stdout.
  2. The speed of each operation must be equal or better. We have spent tons of hours optimizing everything, and every ms counts when you run a code 1,000,000,000x :)
  3. This is probably the hardest one: the operations must use the same equations in the same order. All the code is tested by comparing the result to machine epsilon to ensure nothing changes. Hence, I am pretty sure that if we use different ordering when computing, for example, the quaternion multiplication, it will lead to different results when analyzing the least significant bits.
  4. This is a horrible request, but I will also need it since this package is used for daily tasks related to the space program: I will need to have write access to the repository. Sometimes you need to do something different and spot a bug. Since all operations must be reproducible, I store the envs for all the things to allow for review. Hence, sometimes I must make a quick and very fast change to the repository code.

If you are willing to adapt Quaternions.jl to attend those points, we can try to integrate it with ReferenceFrameRotations.jl :) However, I fully understand if you decide not to since those are not common asks in open-source development.

I am really sorry for those demands, but this package is really crucial for daily work here.

@hyrodium
Copy link
Contributor Author

Thank you for the detailed comment!

Everything must be the same, with no API or breaking changes. This includes, for example, the naming convention for the quaternion elements and the output print to stdout.

This is probably the hardest one: the operations must use the same equations in the same order. All the code is tested by comparing the result to machine epsilon to ensure nothing changes. Hence, I am pretty sure that if we use different ordering when computing, for example, the quaternion multiplication, it will lead to different results when analyzing the least significant bits.

I think we will not change Quaternions.jl just for adapting ReferenceFrameRotations.jl, so let's keep avoiding the migration of Quaternion.

However, in some cases, Quaternions.jl have better performance on overflow/underflow. Maybe there are more cases that one package has better performance than the other, so comparing these implementations and tests will be helpful to both packages.

julia> import Quaternions

julia> import ReferenceFrameRotations

julia> q1 = Quaternions.Quaternion(1e-300,0,0,0)
Quaternions.QuaternionF64(1.0e-300, 0.0, 0.0, 0.0)

julia> q2 = ReferenceFrameRotations.Quaternion(1e-300,0,0,0)
Quaternion{Float64}:
  + 1.0e-300 + 0.0i + 0.0j + 0.0k

julia> abs(q1)
1.0e-300

julia> norm(q2)
0.0

@ronisbr
Copy link
Member

ronisbr commented Jan 17, 2023

However, in some cases, Quaternions.jl have better performance on overflow/underflow. Maybe there are more cases that one package has better performance than the other, so comparing these implementations and tests will be helpful to both packages.

Very good! Thanks for letting me know. I will really consider Quaternion.jl when we finish the current satellite project and have a window for big migrations :)

@hyrodium
Copy link
Contributor Author

I will really consider Quaternion.jl when we finish the current satellite project and have a window for big migrations :)

Great!! Please reopen this issue if you need my help 🤗

@ronisbr
Copy link
Member

ronisbr commented Jan 17, 2023

Sure! Thank you very much!

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

No branches or pull requests

2 participants