search

JuliaRobotics / IncrementalInference.jl

Clique recycling non-Gaussian (multi-modal) factor graph solver; also see Caesar.jl.
MIT License
72 stars 21 forks source link

Manifest VariableTypes (Singleton model) and new FMD #783

Closed GearsAD closed 3 years ago

GearsAD commented 4 years ago

Issue Summary

History:

variableTypesofttype was added to DFGVariable DataLevel2 to solve two problems, then DFGVariableSummary (DataLevel1) came along and is exposing two competing needs which are being wrestled out as softtypename.

Requirements

Related Issues

Plan of Action

Can split these into more issues once a design decision is made.

Original Question

We need to fix all softtypes to have a default constructors, and take out all state information.

Types with issues:

struct ContinuousMultivariate{T1 <: Tuple} <: InferenceVariable
  dims::Int
  manifolds::T1
  ContinuousMultivariate{T}() where {T} = new()
  ContinuousMultivariate{T}(x::Int; manifolds::T=(:Euclid,)) where {T <: Tuple} = new(x, manifolds)
end
mutable struct DynPose2 <: IncrementalInference.InferenceVariable
  ut::Int64 # microsecond time
  dims::Int
  manifolds::Tuple{Symbol,Symbol,Symbol,Symbol,Symbol}
  DynPose2(;ut::Int64=-9999999999) = new(ut, 5, (:Euclid,:Euclid,:Circular,:Euclid,:Euclid))
end
struct ContinuousScalar <: InferenceVariable
  dims::Int
  manifolds::Tuple{Symbol}
  ContinuousScalar(;manifolds::Tuple{Symbol}=(:Euclid,)) = new(1, manifolds)
end

DF EDIT, a related issue is how to make it easy for the user to add their own data types to the default factors. Softtype with state solved that problem, but making it a manifest or hardtype removes that option. SC: I suggest we add smallData to factors.

Affie commented 4 years ago

Just for reference: https://docs.julialang.org/en/v1/manual/types/#man-singleton-types-1

Affie commented 4 years ago

Singleton types can work with current Pose2 for example:

struct Pose2 <: IncrementalInference.InferenceVariable
  dims::Int
  manifolds::Tuple{Symbol,Symbol,Symbol}
  Pose2() = new(3, (:Euclid, :Euclid, :Circular))
end

as

struct Pose2 end
getdims(::Pose2) = 3
getmanifolds(::Pose2) = (:Euclid, :Euclid, :Circular)
dehann commented 4 years ago

Also see JuliaRobotics/RoME.jl#244 on standardizing with Manifolds.jl.

dehann commented 4 years ago

Copy from Slack, @GearsAD asked if this was workable:

# TODO: Confirm that we can switch this out, instead of retrieving the complete variable.
# Original getSofttype(getVariable(dfg,lbl), solvekey)
getSofttype(v::DFGVariable) = typeof(v).parameters[1]()
Affie commented 4 years ago

FIY, I don't know if the introspection is the best way in: getSofttype(v::DFGVariable) = typeof(v).parameters[1]() rather use: getSofttype(::DFGVariable{T}) where T = T()

Edit: added the brackets, the 2 functions are equivalent. This is just to show a cleaner way to write the same function.

DF edit -- agree it's cleaner, sorry I misunderstood how T was being used.
For reference: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/blob/cdcc0dd0a77fc22f163aefb61db3b105a09746cf/src/entities/DFGVariable.jl#L28

Affie commented 4 years ago

I wanted the same concept in the summary graphs and basically did it with a warning: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/blob/6b903b79b15c6754191cfb2b673dd89d1eb97e2c/src/services/DFGVariable.jl#L293-L296

also see: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/237

dehann commented 4 years ago

rather use dispatch: getSofttype(::DFGVariable{T}) where T = T

that would be a hard type -- that's an even bigger discussion. Going softtype allows a bit more freedom and does not require enumeration of all the factor permutations.

EDIT see edit above: https://github.com/JuliaRobotics/IncrementalInference.jl/issues/783#issuecomment-660207623

a related issue is how to make it easy for the user to add their own data types to the default factors. Softtype with state solved that problem, but making it a manifest or hardtype removes that option.

Affie commented 4 years ago

This issue is closely related to the possible future use of a 'manifold type'. I don't quite understand the whole hard-type soft-type thing yet, but that's beside the point. I can see how ContinuousMultivariate can maybe be referred to as a soft-type. The other types are closely related to traits of the value array (it tells the algorithm how to handle the values).

So is the core of the issue:

Split Data Proposals

some options off the top of my head:

In the visualization use case, I needed the type to visualize it. There are only so many options that can currently be visualised. I only wanted Gaussian with a point, so PPE, but its useless without the softtype to tell you what is going on. With "summary graph", softtypename was enough for the basic types such as Pose2 and Point2 (The singleton types). The rest is user-specific and hard (impossible) to make a generic visualiser for.

Affie commented 4 years ago

that would be a hard type -- that's an even bigger discussion. Going softtype allows a bit more freedom and does not require enumeration of all the factor permutations.

See edit above in the comment; to make reason a bit clearer and fix brackets.

@dehann, I don't know if I understand your comment correctly, maybe elaborate if it's still applicable after the edit?

Affie commented 4 years ago

@GearsAD if there are not too many, perhaps this idea could work for the short term. The union is not the best and just to illustrate the idea.

getSofttype(::DFGVariable{T}) where T <:  Union{"all the singleton types"} = T()
getSofttype(::DFGVariable{T}) where T <:  Union{"all the rest"} = ...as it was before...
dehann commented 4 years ago

Hi, trying to simplify and keep short issue -- see summary in first comment edit: https://github.com/JuliaRobotics/IncrementalInference.jl/issues/783#issue-659285005

I don't know if I understand your comment correctly, maybe elaborate if it's still applicable after the edit?

If I understood you right, here is my comment update: https://github.com/JuliaRobotics/IncrementalInference.jl/issues/783#issuecomment-660207623

How do we add user data to VariableNodeData to be used in the solver? Do we split out the user solver data?

I added my vote in first comment, and edited the Johan's suggestion above to add my support for which way forward.

GearsAD commented 4 years ago

Singleton types can work with current Pose2 for example:

struct Pose2 <: IncrementalInference.InferenceVariable
  dims::Int
  manifolds::Tuple{Symbol,Symbol,Symbol}
  Pose2() = new(3, (:Euclid, :Euclid, :Circular))
end

as

struct Pose2 end
getdims(::Pose2) = 3
getmanifolds(::Pose2) = (:Euclid, :Euclid, :Circular)

Right, so this is a more explicit version of the same idea, although in this case we'd be using the singleton form of Pose2? That sounds like a reasonable way to enforce that it doesn't retain state.

GearsAD commented 4 years ago

As in the edits in the initial description:

dehann commented 4 years ago

it may be slow (is it really though?)

if we use the inheritance approach Johan suggests then we force the user to keep type safety (if I'm understanding the use right). I was trying to do the same thing by using softtype originally, but now we need to do more things with it -- so this is a feature expansion process actually.

type stability is a major performance thing, so we do need a reasonable approach to keeping that for normal use. An intermediate user should be able to augment their own data (like the radar images) and still have type safety. I think if a new user is just trying something, then combo of slow performance and documentation should help them advance towards an intermediate user (i.e. using user type for cached data).

I'm just thinking we may want to serialize and save it somewhere

That goes back to smalldata. How much should we force type stability in small data?

GearsAD commented 4 years ago

Currently we're proposing smalldata is like Union{Int, Float64, String, Vector{Int}, Vector{Float64, Vector{String}}. I think that should be a reasonable amount of flexibility and won't hammer performance?

dehann commented 4 years ago

Oh, that will hammer performance unfortunately, the object containing a field ::Union{...} is the potential problem there (85% sure about that). So either we should think of a way to harden smalldata, or as Johan suggests introduce yet another field "metadata". I'm not too hot on the additional field thing, personally I'd rather just limit the user options on smalldata. However, I think @Affie has something clever up his sleeve to use inheritance with "traits" to build 3rd option here that does a good enough job on both concerns (type safety and flexibility, but usage is one up from novice).

EDIT: functions dispatching on a parameter ::Union{..} or ::Abstract is fine, but challenge comes in with ::Union or ::Abstract type fields -- the latter uses "dynamic dispatch" to interpret on the fly how to work with the data. The earlier case of dispatch on functions can preemptively do type-inference and then statically compile a hardtype function.

Affie commented 4 years ago

Currently we're proposing smalldata is like Union{Int, Float64, String, Vector{Int}, Vector{Float64, Vector{String}}. I think that should be a reasonable amount of flexibility and won't hammer performance?

Slightly off topic:

This will perform way better than a "type stable" string that has to be parsed every time.

If you limit the instability with other techniques such as adding type annotations and/or breaking up functions, it may be a good trade-off.

For example:

julia> d = Dict{Symbol, Any}();
julia> push!(d, :a=>5);
julia> push!(d, :b=>5.0)
Dict{Symbol,Any} with 2 entries:
  :a => 5
  :b => 5.0

julia> function sumD(d)
           return d[:a]::Int + d[:b]::Float64
       end
sumD (generic function with 1 method)

julia> @code_warntype sumD(d)
Variables
  #self#::Core.Compiler.Const(sumD, false)
  d::Dict{Symbol,Any}

Body::Float64
1 ─ %1 = Base.getindex(d, :a)::Any
│   %2 = Core.typeassert(%1, Main.Int)::Int64
│   %3 = Base.getindex(d, :b)::Any
│   %4 = Core.typeassert(%3, Main.Float64)::Float64
│   %5 = (%2 + %4)::Float64
└──      return %5
Affie commented 4 years ago

Right, so this is a more explicit version of the same idea, although in this case we'd be using the singleton form of Pose2? That sounds like a reasonable way to enforce that it doesn't retain state.

The reason to rather use a singleton is it can only have one instance. eg 

julia> Pose2() === Pose2()
true

I think the Singleton model from @Affie's suggestion would enforce the stateless structure, I'm just thinking we may want to serialize and save it somewhere. If we go with that, it's hardcoded so we can't do that.

Currently, Pose2 requires RoME to use, so I don't think it would make a difference. It makes it easier as you can just serialize the name, ie "Pose2" and not any fields. So getSofttype(::DFGVariable{T}) where T = T() will still work.

Affie commented 4 years ago

Breadcrumb for function name: issingletontype

Affie commented 4 years ago

@dehann, is this the way it's added to factors:

https://github.com/JuliaRobotics/IncrementalInference.jl/blob/e9351e7140c3fcd44676cb6f65637f89f06f72ac/src/FactorGraphTypes.jl#L147-L160

So variableuserdata = softtype And small data also? although I can't see where its copied in.

And related to performance #383 and the "Vector of abstract".

We can make it more type-stable by variableuserdata::Vector{T} where T<: InferenceVariable or something like that.

Affie commented 4 years ago

Perhaps we can split it completely as a new abstract type? (But you know me, I like abstracting everything)

struct  MyTimeVaribleUserData <: AbstractVariableUserSolverData
   utime::Int
end

#and have

variableuserdata::Vector{T} where T<: AbstractVariableUserSolverData

If the options are a Dictionary (like small data) or a Struct, my vote is for struct since this will be called a lot and we want it in the stack.

Perhaps, we can copy the user data to a NamedTuple (if it's immutable) for use in the calculations

PS. I'm just writing a bunch of options as a brainstorming exercise when I think of them.

Affie commented 3 years ago

@dehann, why is utime not a measurement in the factor?

GearsAD commented 3 years ago

So I think in an ideal world we should remove FactorMetadata and head in this direction:

function (vp2vp2::VelPoint2VelPoint2{D})(
                res::Array{Float64},
                variables::Vector{DFGVariable},
                idx::Int,
                meas::Tuple,
                Xi::Array{Float64,2},
                Xj::Array{Float64,2}  ) where D
  #
  z = meas[1][:,idx]
  xi, xj = Xi[:,idx], Xj[:,idx]
  # change in time from microseconds with DynPoint2(ut=1_000_000) to seconds
  dt = (variables[2].timestamp - variables[1].timestamp)*1e-6   # roughly the intended use of userdata
...

DF EDIT, FactorMetadata has to remain for other reasons. This problem has been resolved by changing from softtype to a hard type VariableType

dehann commented 3 years ago

From the user perspective (DFG and IIF will have to do this internally somehow)


struct FactorMetadata_NEW{USER}
  cacheData::Vector{USER}
  fullVariable::Vector{DFGVariable}
  variablelist::Vector{Symbol} # obsolete
  solvefor::Symbol
  factoruserdata
  dbg::Bool
end

FactorMetadata_NEW(x...;kw...) = FactorMetadata_NEW{Nothing}(nothing, x...;kw...) 

# so naive replacement for timestamp is to use

function (::AwesomeFactor)(..., fmd::FactorMetadata, ...)

  t1 = getTimestamp(fmd.fullVariable[1])
  t2 = getTimestamp(fmd.fullVariable[2])

  # do math
end

# so intermediate user replacement for timestamp is to use

struct MyDynPoseCache
  timestamp::Int64
end

## not callback, rather dispatch
# this must happen either before each solve or before each convolution
# default in IIF
# based on USER, cacheData::Vector{USER}
prepCache(::Type{Nothing}, ::DFGVariable) = nothing

# user must do this, and IIF will call as standard procedure before solving
import IncrementalInference: prepCache # need better name
prepCache(::Type{MyDynPoseCache}, ::DFGVariable) = getTimestamp(fullVariable)

function (::AwesomeFactor)(..., fmd::FactorMetadata, ...)
  t1 = fmd.cacheData[1].timestamp
  t2 = fmd.cacheData[2].timestamp

  # do math
end

## What the heck, let me do it myself...

struct MyDynPose2Factor
  # .. do it all inthe factor
end

EDIT, DF I missed this before, sorry:

So I think in an ideal world we should remove FactorMetadata and head in this direction:

We do need other information like which variable is being solved for etc. I think we have a workable solution now, where the variables are included in the FactorMetadata{T} structure, alonside a Templated type T for user upgrade option for high performance hard-type approach -- i.e. an optimization after getting it working with direct hack on some variable accessor (possibly even data blob retrieval in the computation hot-loop ... yucky but helpful for quick testing). The optimization using T would allow the user to cache say a blob retrieval once, and then be fast.

Ground rule though, the factor operations on variable metadata should be totally immutable. The only way for the hot-loop to update the variables is to modify the belief numerical parameters passed in, i.e. 

meas::Tuple,
Xi::Array{Float64,2},
Xj::Array{Float64,2}
...
dehann commented 3 years ago

the prepCache function (we still need a better name for it) will likely be called around here (im like 70% sure): see FactorMetadata: https://github.com/JuliaRobotics/IncrementalInference.jl/issues/783#issuecomment-661899977

Affie commented 3 years ago

Is it ok to have continuous scalar only represent Euclid, ie ContinuousScalar() = new(1, (:Euclid,))

struct ContinuousScalar <: InferenceVariable
  dims::Int
  manifolds::Tuple{Symbol}
  ContinuousScalar(;manifolds::Tuple{Symbol}=(:Euclid,)) = new(1, manifolds)
end

Since we have Sphere1 for (:Circular,)

struct Sphere1 <: InferenceVariable
  dims::Int
  manifolds::Tuple{Symbol}
  Sphere1() = new(1, (:Circular,))
end

We can also do ContinuousMultivariate as all Euclidian

julia> struct ContinuousMultivariateEuclid{N} <: InferenceVariable end
julia> function Base.getproperty(::ContinuousMultivariateEuclid{N}, f::Symbol) where N
         if f == :dims
           return N
         elseif f == :manifolds
           return ntuple(i -> :Euclid, N) 
         end
       end

julia> a = ContinuousMultivariateEuclid{4}()
julia> a.dims
4
julia> a.manifolds
(:Euclid, :Euclid, :Euclid, :Euclid)
dehann commented 3 years ago

Hi, yes I think thats fine thanks -- we can later add things like ContinuousCircular?

dehann commented 3 years ago

i like the ::ContinuousMultivariateEuclid{N} since that is similar to ::NTuple{N} and ::Array{N}

dehann commented 3 years ago

Oh, just a correction :Euclid not :euclid. This will all become part of the JuliaRobotics/RoME.jl#244 Manifold.jl consolidation story.

Affie commented 3 years ago

I wrote down a trait-based example we can look at using the softtypes for other things such as visualization. 

abstract type VisualizerTrait end
struct SE{N} <: VisualizerTrait end
struct Cartesian{N} <: VisualizerTrait end
VisualizerTrait(::T) where T = error("Type $T doesn't implement the VisualizerTrait")

VisualizerTrait(::Pose2) = SE{2}()
VisualizerTrait(::Pose3) = SE{3}()
VisualizerTrait(::Point2) = Cartesian{2}()
VisualizerTrait(::Point3) = Cartesian{3}()

visualize(x) = visualize(VisualizerTrait(x), x)
visualize(::SE{N}, x) where N = @info "Visualizing SE$N..."
visualize(::Cartesian{N}, x) where N = @info "Visualizing Cartesian $N..."

Xs = [Pose2(), Pose3(), Point2(), Point3()]

julia> visualize.(Xs)
[ Info: Visualizing SE2...
[ Info: Visualizing SE3...
[ Info: Visualizing Cartesian 2...
[ Info: Visualizing Cartesian 3...
dehann commented 3 years ago

i like those traits!

Affie commented 3 years ago

Added advantage, micro performance improvement. It might be negligible but this way the constant propagation can be used:

#the 2 versions of:
@code_lowered getDims(p3)
# old
CodeInfo(
1 ─ %1 = Base.getproperty(p, :dims)
└──      return %1
)
# new
CodeInfo(
1 ─     return 6
)
dehann commented 3 years ago

nice!

dehann commented 3 years ago

Decision for #784 made, consolidating work effort in this issue.

Affie commented 3 years ago

Partially addressed by #823 - with full variables. Deprecation stared to change softtypes to singletons

dehann commented 3 years ago

lol https://github.com/JuliaRobotics/RoME.jl/issues/62

dehann commented 3 years ago

see JuliaRobotics/DistributedFactorGraphs.jl#603

Affie commented 3 years ago

Stared the deprecation process in #841.

dehann commented 3 years ago

Too big a change to also include in v0.17.0, punting down one to v0.18.0 since this will require a breaking change upstream from DFG v0.11.0 which is not available yet.

dehann commented 3 years ago

xref #825