Tricky interface for `AffineMap(::LinearMap, ::Vector)`

[RomeoV]

I just encountered a very tricky silent bug while slinging around AffineMap, LinearMap, Transformation etc.

In summary, I have some 3D rotation matrices stored as LinearMap and tried to combine it with a translation stored as Vector. Spot the error in this code:

using CoordinateTransformations, Rotations
rot = LinearMap(RotY(1/2*pi))
# ...
loc = rand(3)
# ...
pose_map = AffineMap(rot, loc)
# ...
f(pmap::AffineMap) = ...
@test f(pose_map) == test_value # <- compiles just fine, but result completely wrong

It turns out that AffineMap(::LinearMap, ::Vector) actually calls an overloaded function AffineMap(::Transformation, ::Any) and returns an AffineMap just fine, but with AffineMap.v == zeros(3)! https://github.com/JuliaGeometry/CoordinateTransformations.jl/blob/b97c74a35c6835f6b015440cb4af6298824dbe1d/src/affine.jl#L121-L125 I.e.

# continued
@assert pose_map.linear == rot.linear # true
@assert pose_map.translation == loc # false. instead
@assert pose_map.translation == zeros(length(loc)) # <- 🤯

This is a super tricky bug, as it's completely silent, and occurs from a "misuse" of the interface that is very subtle.

Perhaps it would make sense to rename the function

function AffineMap(trans::Transformation, x0)
    dT = transform_deriv(trans, x0)
    Tx = trans(x0)
    AffineMap(dT, Tx - dT*x0)
end

to something like AffineMapApprox (or something similar), although I realize that would be a breaking change. Alternatively, we could overload AffineMap(::LinearTransformation, ::Any), e.g. giving a warning like

@warn "AffineMap(rot::LinearTransformation, ::Any) might not do what you want. Try AffineMap(rot.linear, x) instead."

[andyferris]

Hmm, I see. It probably makes sense to rename the linear approximation to something more explicit. Add a deprecation for that, and allow AffineMap(::LinearTransformation, ::Any) to do the "sensible thing", and remove the deprecated method sometime in the future.

Are there any consumers of this method with an opinion? (Actually - is the linear approximation "wrong" w.r.t. the offset component anyway? If we want a linear approximation around a point wouldn't we want a LinearMap?)

@RomeoV would you be willing to contribute a PR?

[RomeoV]

Happy to make a PR after a little more discussion on the interface. Another interface possibility just occurred to me:
I think having AffineMap implicitly do an approximation should imo be changed no matter what. Then, we still have a bunch of possiblilities for the AffineMap interface, including

AffineMap(::LinearTransformation, ::Any)
AffineMap(::AbstractMatrix, ::Translation)
AffineMap(::AbstractMatrix, ::Vector)

and a bunch of other combinations. Since AffineTransform is always evaluated as

https://github.com/JuliaGeometry/CoordinateTransformations.jl/blob/b97c74a35c6835f6b015440cb4af6298824dbe1d/src/affine.jl#L104-L109

I propose we instead change the definition of AffineMap to

struct AffineMap{M<:LinearMap, V<:Translation} <: AbstractAffineMap
    linear::M
    translation::V
end

(note the restriction in the template paramters) together with a constructor

AffineMap(M, V) = AffineMap(LinearMap(M), Translation(V))

and then simply

(map::AffineMap)(x) = (map.translation \circ map.linear)(x)

Would that be fine? I have to look into the exact mechanics of the deprecation mechanism to understand if that makes sense in this context.

[RomeoV]

Also, is there any reason we don't restrict the inner type of LinearMap{M} to Union{AbstractMatrix, Number} (and similar for Translation)? In fact, the docstring specifically reads

https://github.com/JuliaGeometry/CoordinateTransformations.jl/blob/b97c74a35c6835f6b015440cb4af6298824dbe1d/src/affine.jl#L37-L43

i.e. it refers to "a matrix-like object", i.e. an AbstractMatrix? This change would further strengthen the interface against accidental misuse.