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

[kunzaatko]

[!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)

[timholy]

I'm not sure I understand your concerns. Are you conflating construction and convert?

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

julia> convert(RotMatrix, π/4)
ERROR: MethodError: Cannot `convert` an object of type
  Float64 to an object of type
  RotMatrix

Closest candidates are:
  convert(::Type{SA}, ::AbstractArray) where SA<:StaticArraysCore.StaticArray
   @ StaticArrays ~/.julia/packages/StaticArrays/MTYJP/src/convert.jl:207
  convert(::Type{SA}, ::StaticArraysCore.StaticArray{S}) where {SA<:StaticArraysCore.StaticArray, S<:Tuple}
   @ StaticArrays ~/.julia/packages/StaticArrays/MTYJP/src/convert.jl:185
  convert(::Type{SA}, ::SA) where SA<:StaticArraysCore.StaticArray
   @ StaticArrays ~/.julia/packages/StaticArrays/MTYJP/src/convert.jl:191
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[5]:1

To me, that seems appropriate.

[kunzaatko]

Are you conflating construction and convert?

I don't think that I am.

It may be kind of a nitpick, but for the reasons stated above, I do not believe the constructor of RotMatrix for a single argument θ should exist.

Are my arguments why I believe that this is the case clear? Do you understand what I am pointing at?

[timholy]

Maybe others understand your concerns, but to me the first three are not clear.

[kunzaatko]

• RotMatrix type is "raw" and should be used only if you have the "raw" fields of the rotation i.e. the rotation matrix.

Here I mean that you should be able to construct a RotMatrix only if you have the exact field of the struct i.e. the matrix of rotation. There is really no reason for the 2D rotation to have precedence over the other dimensionalities. It is just a wrapper/marker type for a 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.

When someone discovers the RotMatrix(θ) method creating a RotMatrix{2}, he would (as I did) believe that when you call RotMatrix(α, β, γ) that it would construct RotMatrix{3} from the Euler angles α, β, γ. However that is not a method. (I know that this method cannot exist since one of the conventions would need to be arbitrarily selected...).

• The existence of the parametrized method falsely tips the user that there exists something as Rotations.params(RotMatrix(θ)) which does not.

Since parameters are passable to the constructor and the Rotations.params method exists, there is reason to believe that what works for any other rotation type would also work for this one, i.e. when you call Rotations.params(RotMatrix(θ)) you would retrieve the parameter θ. Instead the RotMatrix struct does not define the Rotations.params interface.

julia> rot = RotMatrix(π/3)
2×2 RotMatrix2{Float64} with indices SOneTo(2)×SOneTo(2):
 0.5       -0.866025
 0.866025   0.5

julia> Rotations.params(rot)
ERROR: MethodError: no method matching params(::RotMatrix2{Float64})
The function `params` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  params(::AngleAxis)
   @ Rotations ~/Dev/julia/Rotations/src/angleaxis_types.jl:40
  params(::RotZYX)
   @ Rotations ~/Dev/julia/Rotations/src/euler_types.jl:551
  params(::RotYZX)
   @ Rotations ~/Dev/julia/Rotations/src/euler_types.jl:551
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

Is what I mean clear now?

[timholy]

Much clearer. For the first, presumably your only objection is to https://github.com/JuliaGeometry/Rotations.jl/blob/cee8b77d1e44dd464b40f44f57364dbfff79c42c/src/core_types.jl#L103

and not the method below it?

[kunzaatko]

I believe that neither should exist and it should be left to Angle2d instead but removing this method would be adequate IMHO even if you don't think that the other should be.