Review abstract types and names used in IIF that is exported from DFG

[Affie]

To review

• InferenceVariable
• AbstractRelativeMinimize
• AbstractManifoldMinimize
• pack/unpackDistribution
• SamplableBelief
• AbstractPrior -> PriorObservation
• AbstractRelative -> RelativeObservation
• TODO fill in more..

other

• InferenceType used in SDK should be removed

[Affie]

1. I'm happy with AbstractPrior and AbstractRelative, but we can add Observation if need for clarity eg. AbstractRelativeObservation

2. The noun [pack/unpack]Distribution and SamplableBelief refers to the same thing as far as I can tell, so should we combine them? Distribution in SDK is currently a duplicate of PackedSamplableBelief and should be consolidated and deprecated.

3. AbstractRelativeMinimize and AbstractManifoldMinimize should maybe become traits to be expandable, eg. getSolverMethod(::Pose2Pose2) = ManifoldMinimization() with the default to AbstractManifoldMinimize - I think we are moving to only one method in any case? But traits open us up to change easier in the future.

4. Is InferenceVariable clear enough for new users (used in declaring variables eg. struct Position{N} <: InferenceVariable end) and does it match with the new State noun? InferenceType in the SDK refers to the factor type adding to the confusion. VariableType is used in the SDK for the same thing. Variable type is also confusing as we have a few variable types (VariableCompute, VariableDFG, etc.) . Maybe [Abstract]VariableStateType?

5. There is currently a mix of InferenceVariables with a dimension parameter and without, we should maybe just formalize to always have a parameter (or never as it might just be Position{N}), eg. Pose2 becomes Pose{2} with alias as well. InferenceVariable would become InferenceVariable{N<:Integer} where N is always the dimension.

[dehann]

I'm happy with AbstractPrior and AbstractRelative, but we can add Observation if need for clarity eg. AbstractRelativeObservation

+1 for adding Observation, but why not just use RelativeObservation and PriorObservation. I feel the abstract designator is already available, so don't really need to repeat it in the signature -- i.e. isabstracttype(RelativeObservation) --> true.

The noun [pack/unpack]Distribution and SamplableBelief refers to the same thing as far as I can tell, so should we combine them? Distribution in SDK is currently a duplicate of PackedSamplableBelief and should be consolidated and deprecated.

+1 to remove the duplication, how about SerdeDistribution?
serde is Rust speak, reduced from ser/de, reduced from serialization/deserialization. Rust has important library serde and serde_json. That "noun" has become pretty useful in my day-speak.

AbstractRelativeMinimize and AbstractManifoldMinimize should maybe become traits to be expandable, eg. getSolverMethod(::Pose2Pose2) = ManifoldMinimization() with the default to AbstractManifoldMinimize -

Yes, +1 agree.

I think we are moving to only one method in any case? But traits open us up to change easier in the future.

My experience has been that there will always be another optimization library popping up somewhere, so probably we'll end up with one default, but would like to keep the option open of rapidly integrating a new optimization library in the future.

Is InferenceVariable clear enough for new users (used in declaring variables eg. struct Position{N} <: InferenceVariable end) and does it match with the new State noun? InferenceType in the SDK refers to the factor type adding to the confusion. VariableType is used in the SDK for the same thing. Variable type is also confusing as we have a few variable types (VariableCompute, VariableDFG, etc.) . Maybe [Abstract]VariableStateType?

Oh, dang that is confusing.

So my mental model has been VariableDataLevel <: DataLevel, but even that is weak and we don't have those names written out yet either. Do we need a DataLevel name for these as explicit types today - i.e. VariableCompute <: VariableDataLevel? I'm starting to think yes we do (maybe it's already there). Similarly FactorDataLevel <: DataLevel.

Then, to help newcomers (and old school GNC folks) find their way in new code, how about variables use abstract type StateEstimateVariable end -- i.e. struct Position{N} <: StateEstimateVariable. That order lines up with much existing literature on specifically "state estimate [for position] variables". The optimization community literature has "decision variables", but that's probably not good for our use since we're likely to rather talk about "decision taken from" control loops / actions taken from perception estimates. Maybe/sorta for the trajectory planning aspects. Either way state/decision variables certainly screams that our old names are bad, i.e. we should remove InferenceType/InferenceVariable/VariableType; also since InferenceType is likely to be replaced by traits as per bullet item 3. above.

There is currently a mix of InferenceVariables with a dimension parameter and without, we should maybe just formalize to always have a parameter (or never as it might just be Position{N}), eg. Pose2 becomes Pose{2} with alias as well.

yes, +1 for Position{N::Int}, Pose{N::Int}, etc.

InferenceVariable would become InferenceVariable{N<:Integer} where N is always the dimen[s]ion.

First reaction is no on this one, let's first get to one abstract type StateEstimateVariable -- I'm not sure we should bake case by case dimensionality into the abstract type.

---

TL;DR; Recap: Pose{N::Int} <: StateEstimateVariable, PriorPosition{N::Int} <: PriorObservation. Non-Julia getVariable returns VariableDFG <: VariableDataLevel, and logic similarly applies over all cases FactorDataLevel/VariableDataLevel <: DataLevel.

I'm also reading these as being defined in DFG, and SDK.jl imports them -- i.e. we are trying to remove all abstract types from SDK.jl and collect them in DFG as best able? Similarly, IIF just imports them from DFG. After this round, next might be sorting out abstracts in AMP too.

[Affie]

From call, we decided to go with just Belief to replace Distribution and SamplableBelief

Pending JSON.jl v1, we are skipping the intermediate structures for PackedSamplableBeliefs with the SDKs to follow the <: Belief naming and constructors as far as possible (eg. FullNormal vs MvNormal) .
Sort out errors on constructors as type will no longer inherit from the SamplableBelief union type.

xref #1133

[Affie]

Make it clear that [Variable]State contains the state estimate in classical methods.

[dehann]

[not good] StateEstimateVariable

@Affie made the case that we don't store state in the [VariableStateType]. e.g. Pose2 does not store the state, that is kept alongside in the Variable -- hence driving the inclusion of word Type (also why we should not use Estimate). Motivations for why .state lives next to the variable type is because we don't just estimate the value of pose2, we also sample, serialize, keep multiple stateLabel super positions, store confidence estimates, we use different algorithms, etc. etc.

So suggested to go with more with something like VariableStateType

[VariableType is bad because ]VariableType is used in the SDK for the same thing. Variable type is also confusing as we have a few variable types (VariableCompute, VariableDFG, etc.)

We like the word State in this abstract type because it refers to the state estimate inside the variable, e.g. Pose2.

We want the word Variable in the abstract type because we anticipate the word state will show in both variables and factors in the future.

[dehann]

Let's vote on abstract type VariableStateType, with usage example: Pose2 <: VariableStateType:

• JT 👍
• DF 👍

[Affie]

On point 5, it is harder to change the implementation to use the generic VariableStateType{N} syntax eg. Pose{N} for DFG v1, we can start with "hardcoded" types eg. Pose2, Pose3 and then alias the types in a minor release without breaking changes.