JuliaGeometry / JuliaGeometry/Rotations.jl
`RotMatrix(θ::Number)` should be a non-method
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@deprecateduntil then.
I believe that the method RotMatrix(θ::Number) should not exist. This is because of four reasons:
RotMatrixtype 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
RotMatrixexists, 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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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