Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should a Lazy wrapper for Manifolds.jl exist? #450

Open
Affie opened this issue Jun 14, 2021 · 7 comments
Open

Should a Lazy wrapper for Manifolds.jl exist? #450

Affie opened this issue Jun 14, 2021 · 7 comments

Comments

@Affie
Copy link
Member

Affie commented Jun 14, 2021

Hi @Affie, i'm having doubts about this whole approach. I'm now thinking we should not be building types like SE and se at all.

I think we should drastically reduce the amount of duplication with Manifolds.jl and just simply use
VND.val::Vector{ProductRepr...}

and implement just RoME.oplus(::Pose3Pose3, p1,p2) which follows the same style as Manifolds.jl. That way we avoid the exp/retract issue we had in 366

I really want to avoid duplicating types, abstracts and API design from Manifolds. I'm actually thinking we should not implement a Lazy wrapper at all for the next couple of weeks and simply add the oplus functions in RoME until we have a better idea of what does and doesn't work. All we doing is dispatch against func(::AbstractFactor,...). E.g.

v = getCoordinates(Pose3, p1)

which is easily overloaded for

v_arr = getPointCoordinates(fg, :x1) # which we already know is a Pose2 or InertialPose3 etc

Originally posted by @dehann in dehann/TransformUtils.jl#45 (comment)

@Affie
Copy link
Member Author

Affie commented Jun 14, 2021

Option A Pose as is
The main reason I like a wrapper is to give concrete types as there is no type distinction between representations of the group and the algebra.

RoME.oplus(::Pose3Pose3, p1,p2) which follows the same style as Manifolds.jl

I wouldn't implement this without concrete types on p1 and p2.
as you also have
RoME.oplus(::Pose3Pose3, p,X) and RoME.oplus(::Pose3Pose3, X, p)
And oplus can then also be generalized to any lie group RoME.oplus(M::AbstractGroupManifold, p1,p2)

The second reason, although not really that important, is it makes it slightly easier for me to use.
Take PosePose as an example:

function PosePose(ⁱmⱼ::SE{N}, ʷxᵢ::SE{N}, ʷxⱼ::SE{N}) where N
    ʷT̂ⱼ = ʷxᵢ  ⁱmⱼ
    ʲT̂ⱼ = ʷxⱼ \ ʷT̂ⱼ
    X = log(ʲT̂ⱼ)
    return vee(X)
end

The equivalent will be something like this to implement Pose2 and Pose3:

function Pose2Pose2(ⁱmⱼ, ʷxᵢ, ʷxⱼ)
    M = SpecialEuclidean(2)
    ʷT̂ⱼ = compose(M, ʷxᵢ, ⁱmⱼ)
    ʲT̂ⱼ = compose(M, inv(M, ʷxⱼ), ʷT̂ⱼ)
    X = log(M, identity(M,ʲT̂ⱼ), ʲT̂ⱼ)
    return vee(M, identity(M,ʲT̂ⱼ), X)
end

function Pose3Pose3(ⁱmⱼ, ʷxᵢ, ʷxⱼ)
    M = SpecialEuclidean(3)
    ʷT̂ⱼ = compose(M, ʷxᵢ, ⁱmⱼ)
    ʲT̂ⱼ = compose(M, inv(M, ʷxⱼ), ʷT̂ⱼ)
    X = log(M, identity(M,ʲT̂ⱼ), ʲT̂ⱼ)
    return vee(M, identity(M,ʲT̂ⱼ), X)
end

A third reason is the constructors that are currently in TransformUtils. eg. SO3(E::EulerAngles)


That being said, I'm also not completely convinced either and AMP is likely the place that will determine what is needed.

I would also like to explore the possibility of the variable actually being the type. see next comment

@Affie
Copy link
Member Author

Affie commented Jun 15, 2021

Option B - T <: InferenceVariable to store a manifold point and manifold

struct Pose{M <: Manifolds.AbstractGroupManifold, T}
  value::T 
end
mutable struct VariableNodeData{T<:InferenceVariable}#, P, B}
    "Vector of a point on the manifold, Eg Pose{SE2}(x,y,θ)"
    val::Vector{T}
...

Option C - T <: InferenceVariable to store types only
In a similar way, the data type and manifold can be stored with the InferenceVariable.

abstract type InferenceVariable{M<:AbstractManifold, T} end


struct Pose{M<:SpecialEuclidean, T} <: InferenceVariable{M, T} end

const Pose2 = Pose{SpecialEuclidean{2}, ProductRepr{Tuple{SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}}
const Pose3 = Pose{SpecialEuclidean{3}, ProductRepr{Tuple{SVector{3, Float64}, SMatrix{3, 3, Float64, 9}}}}

var_type = Pose{SpecialEuclidean{2}, SVector{3,Float64}}()

getPointType(::Type{<:InferenceVariable{M, T}}) where {M,T} = T
getManifoldType(::Type{<:InferenceVariable{M, T}}) where {M,T} = M

vnd.val::Vector{getPointType(var_type)}()

@Affie
Copy link
Member Author

Affie commented Jun 16, 2021

Rabbit hole: To capture it somewhere:
As expected but still cool to see, all factors from a point to a point on group manifolds will look exactly the same (also priors)

These are based on f:M→ℝ

#manifolds point to manifold point factor _d uses distance function
function PointPoint_d(m, p, q)
    q̂ = p  m
    return distance(q, q̂)
end
# manifold prior factor _d uses distance function
function Prior_d(meas, p)		
    return distance(meas, p)
end

or not using LazyLieManifolds.

function PointPoint_d(M::AbstractGroupManifold, m, p, q)
    q̂ = compose(M, p, m)
    return distance(M, q, q̂)
end
Pose2Pose2(m, p, q) = PointPoint_d(SpecialEuclidean(2), m, p, q)
Pose3Pose3(m, p, q) = PointPoint_d(SpecialEuclidean(3), m, p, q)
Point2Point2(m, p, q) = PointPoint_d(TranslationGroup(2), m, p, q)
Point3Point3(m, p, q) = PointPoint_d(TranslationGroup(3), m, p, q)

@dehann
Copy link
Member

dehann commented Jun 17, 2021

Option D

Keep DFG.@defVariable roughly unchanged (adding only small modifications) so that these functions remain in use:

getDimension(::Manifold)
getManifold(::InferenceVariable)

This resolves through dispatch how "Manifold type" information is retrieved (i.e. getManifold(Pose3)) -- note, not the "point/element type" information. Must resolve "point type" needs separately.

For serialization, we expressly decided against any value information inside VariableType, which immediately prevents option A and B (JuliaRobotics/IncrementalInference.jl#783, and JuliaRobotics/DistributedFactorGraphs.jl#603).

Serialization of full manifold types as part of variable types for Option C is also hard, however, an easy getout would be to just use the getManifolds(Pose3) dispatch as well and not explicitly including "manifold type" and "point type" information into the "variable type" of what we call Pose3.

@Affie and I spoke on the phone, and a workable solution for resolving "point type" information is to include this via:

@defVariable Pose3 SpecialEuclidean(3) ProductRepr(MVector3.., MMatrix33...)

i.e. both "manifold type" and "point type" information is provided by the user when creating our "variable type". Users can then more easily create as many "variable types" as they want in the Main context at runtime (super important). We have to limit how much work a user needs to do for making new variables and factors (if it's too hard, folks won't use the code -- also why I'm against new manifold types).

Most important though, is to NOT require other new abstract definitions or wrapper types for existing Manifolds types. Once we have nonparametric and parametric computations fully consolidated on Manifolds.jl operations (i.e. deprecate TransformUtils.jl), then we can as a later step return with wrappers to help (e.g. LazyLieSE2Point{Float64}):

@defVariable Pose3_EZ Manifolds.SpecialEuclidean(3) LazyLieSE2Point{Float64}

# or even do a new macro
@easyDefVariable Pose3_EZ LazyLieSE2Point{Float64}

But this is only a later step AFTER IIF has been transitioned to Manifolds without adding new types and abstractions.

I'm trying to keep both maximum flexibility and lessons learned inside Manifolds.jl available here (preserving versatility, utility and unity in Julia ecosystem), while also minimizing the changes in IIF/RoME. I understand Options A,B,C above as "already assume wrappers are the way to go". I originally thought we should go that way, but now I'm more convinced that this Option D is the necessary first step -- otherwise we are going to take on way too much work at once, and or stir dissonance among "manifold types".

avoiding type piracy

@Affie
Copy link
Member Author

Affie commented Jun 18, 2021

In summary:

We are going with option D, which is option A but with a Lazy wrapper only implemented at a later stage if needed.
xref: JuliaRobotics/DistributedFactorGraphs.jl#775

In options B and C, the lazy wrapper was implemented in the <:InferenceVariable types. With serialization to remain as is, it sounds like a bad idea.

I'm pretty sure a lazy wrapper (or a place to extend Manifolds.jl) will come in handy later.

@dehann dehann modified the milestones: v0.16.0, v0.x.0 Oct 15, 2021
@dehann
Copy link
Member

dehann commented Oct 15, 2021

The decision for IIF v0.25 was that LazyWrapper is a convenience, and IIF / RoME should foremost first support the same thing Manifolds.jl does, so users can mix and match the ecosystem packages. If we are able to build a convenient lazy wrapper which does not inhibit this earlier requirement, we can slowly work in that direction. At time of writing, I'm less enthusiastic about a LazyWrapper (even though I originally helped promote the idea). At present I am more in support of the design philosophy Manifolds.jl followed.

Linking this text to High Level Design Wiki: https://github.com/JuliaRobotics/Caesar.jl/wiki/High-Level-Requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants