Deprecated UnitQuaternion doesn't work

[bzinberg]

UnitQuaternion is now a deprecated alias for QuatRotation:

Rotations.jl/src/deprecated.jl

Line 2 in 89e4dd4

Base.@deprecate_binding UnitQuaternion QuatRotation true

However, field access doesn't work the same as it did in v1.0.x, as w, x, y, z no longer directly stored as fields in the struct. This makes the behavior of UnitQuaternion in v1.1.0 backward-incompatible with v1.0.x. Given that users of UnitQuaternion will already need to update their codebase, how about we just remove UnitQuaternion entirely?

[bzinberg]

Also, I'm happy that we're using this name 🙂

[andyferris]

I'm happy that we're using this name

Sorry - which name do you prefer?

[bzinberg]

The new one, QuatRotation 🙂

[hyrodium]

Should we add something like this and release v1.1.1?

function Base.getproperty(q::QuatRotation, f::Symbol)
    if f == :w
        getfield(q,:q).s
    elseif f == :x
        getfield(q,:q).v1
    elseif f == :y
        getfield(q,:q).v2
    elseif f == :z
        getfield(q,:q).v3
    else
        getfield(q,f)
    end
end

[hyrodium]

Related: #175 (comment)

[bzinberg]

I'd be in favor of deprecating field access completely -- the indirection of having to say q.q.{s,v1,v2,v3} is both not conceptually important and not something we want to be locked into in future versions of the package. For backward compatibility, as @dehann and @hyrodium suggested, we can overload getproperty for QuatRotation.{w,x,y,z} to log a deprecation warning and return their pre-1.1 equivalents. (By the way @hyrodium, I think you can just say q.q.s rather than getfield(q, :q).s.)

I suggest we define a method

to_quaternion(r::QuatRotation)::Quaternions.Quaternion = r.q

that returns the underlying quaternion. One advantage of this over having only Rotations.params(::QuatRotation)::SVector (though we can have that too) is that the API for how to use the return value is right there in the type signature. And it also frees us up to change the structs under the hood if we later want to.

[hyrodium]

I suggest we define a method..

We already have the following conversion. Isn't this enough?

julia> using Rotations, Quaternions

julia> q = rand(QuatRotation)
3×3 QuatRotation{Float64} with indices SOneTo(3)×SOneTo(3)(Quaternion{Float64}(0.182754, 0.632691, -0.155473, 0.736295, true)):
 -0.132605  -0.465855     0.874869
  0.072389  -0.884858    -0.460202
  0.988522   0.00230565   0.15106

julia> Quaternion(q)
Quaternion{Float64}(0.1827539852714259, 0.6326913349141059, -0.155472992405557, 0.736295324054472, true)

I think this should be documented.

[hyrodium]

Ah, we still missing this converison.

julia> convert(Quaternion, q)
ERROR: MethodError: Cannot `convert` an object of type 
  QuatRotation{Float64} to an object of type 
  Quaternion
Closest candidates are:
  convert(::Type{T}, ::Base.TwicePrecision) where T<:Number at twiceprecision.jl:250
  convert(::Type{T}, ::AbstractChar) where T<:Number at char.jl:180
  convert(::Type{T}, ::CartesianIndex{1}) where T<:Number at multidimensional.jl:136
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[11]:1

[ferrolho]

I have also run into this issue because of a package that I use (MeshCat.jl). What is the recommendation going forward? E.g., what is the ideal way of rewriting this line from Rotations.jl v1.1.0 onwards? Thanks!

[bzinberg]

@ferrolho, I think the most general answer to that is "first coerce (or, once #208 (comment) is implemented, convert) to a Quaternions.Quaternion, then apply whatever Quaternions.jl's answer to your question is." My guess is that a reasonable replacement for the line you linked would be

function js_quaternion(q::UnitQuaternion)
    q_ = Quaternions.Quaternion(q)
    return [q_.v1, q_.v2, q_.v3, q_.s]
end

@hyrodium, it might be worth adding a to_quat method just as a convenience for use cases like this, so that a package like the above wouldn't have to add and import Quaternions at its top level.

[ferrolho]

Thank you, @bzinberg. :) And, FWIW, I agree that a convenient to_quat would be appreciated!

[bzinberg]

oh, and change the name UnitQuaternion to QuatRotation 😉

[hyrodium]

it might be worth adding a to_quat method just as a convenience for use cases like this, so that a package like the above wouldn't have to add and import Quaternions at its top level.

Isn't it enough with Rotations.params(q::QuatRotation)?

[andyferris]

We can always use q.q to get the quaternion, right?

[bzinberg]

We can always use q.q to get the quaternion, right?

"Always," until we want to change the under-the-hood details of QuatRotation (perhaps without wanting to change the public API). For example, we could "always" access the components q.w, etc., until v1.1.0 when they no longer existed. I'm fine with us having field access to q.q be part of the public API, but if we do that, we should keep in mind that future changes to the fields could easily be breaking (which is ok, but we should treat them as such for purposes of semantic versioning). Having an accessor method adds indirection, but gives us more flexibility to later change implementation details while keeping the same API.

Not a strong opinion on this, just sharing my thoughts.

[bzinberg]

Isn't it enough with Rotations.params(q::QuatRotation)?

Yes, in principle that's enough. However, the user has to be careful to note the order of arguments returned by this function (which should be in the docstring and any future changes to it should be considered breaking changes; and there are different orders in widespread use). I like the idea of having a method (or just field access, if we decide to make q.q public API) return a Quaternions.Quaternion because it eliminates the possibility of that particular mistake / decreases user cognitive load, i.m.o.: the semantics of which component is which, is baked into the return type, rather than relying on prose and array indices.

[hyrodium]

However, the user has to be careful to note the order of arguments returned by this function (which should be in the docstring

I agree with that.

and there are different orders in widespread use

I understand, but I think (w,x,y,z) format is more natural than (x,y,z,w) because of consistency with complex numbers.
(q = w+ix+jy+kz, c = x+iy)

I like the idea of having a method (or just field access, if we decide to make q.q public API) return a Quaternions.Quaternion

Hmm, I think Quaternion(::QuatRotation) can be used here.