JuliaGeometry / JuliaGeometry/Rotations.jl

`RotMatrix(θ::Number)` should be a non-method

Open
#298 6 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Julia
Stars
188
Forks
45
PR merge metrics
No merged PRs in 30d

Description

[!NOTE]
This would be a breaking change. It can be done that the method will be labeled with @deprecated until then.

I believe that the method RotMatrix(θ::Number) should not exist. This is because of four reasons:

  • RotMatrix type is "raw" and should be used only if you have the "raw" fields of the rotation i.e. the rotation matrix.
  • the method along with the type name are misleading. On the basis of knowing that the single argument RotMatrix exists, one would believe that the three argument method would be one that uses some default Euler angle convention. Such a method does not exist.
  • The existence of the parametrized method falsely tips the user that there exists something as Rotations.params(RotMatrix(θ)) which does not.
  • Most importantly, it is fully replaceable by Angle2d.

There is a difference though. The Angle2d is lazy since it does not construct the rotation matrix eagerly on construction.

I believe that the best option would be to remove (deprecate) the single argument construction of RotMatrix completely and leave the user to construct such a one argument matrix themselves via conversion:

julia> RotMatrix(Angle2d(π/4))
2×2 RotMatrix2{Float64} with indices SOneTo(2)×SOneTo(2):
 0.707107  -0.707107
 0.707107   0.707107

Note that the conversion is compiled away so this leads to no performance setback

julia> @btime RotMatrix(Angle2d(π/4));
  1.803 ns (0 allocations: 0 bytes)

julia> @btime RotMatrix(π/4);
  1.803 ns (0 allocations: 0 bytes)

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the RotMatrix(θ::Number) constructor entry point and inspect related Angle2d conversion behavior and tests. Determine whether the method should be deprecated first or removed as a breaking change, then verify that constructing through RotMatrix(Angle2d(π/4)) remains correct and allocation-free.

Written by the indexing model from the issue text.

Assessment

Tech stack
julia
Domain
backend-api-design
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.