towards val::Vector{T} #783
Conversation
|
Hi, I also considered it, but as far as i know the identity element is not general on all manifolds, but only for groups. |
Sorry I should clarify, i mean user specifies with Either way, we definitely need something for serialization. I think getCoordinates is more general than requiring users to write dedicated serialization converters for each manifold? Hence the need for an easy Identity element. Besides, I don't think it is that big a difference between writing (might actually even be simpler for more complicated array types): @defVariable SpecialEuclidean(3) ProductRepr([0;0;0.0], [1 0 0; 0 1 0; 0 0 1.0])and @defVariable SpecialEuclidean(3) ProductRepr{Tuple{Vector{Float64}, Matrix{Float64}}}If you really don't like it, can we at least just use it as a transition mechanism, until we find something better than defaulting to |
probably not, and in these cases we can just require users to define their own |
|
I'm going to continue down this path for now, to get IIF and RoME to also work for matrixofcoords = getBelief(fg, :x1) |> getPointsSo I was thinking to use the same |
|
Oh and forgot to mention the tests here DFG should be passing on this PR at current commit 6e253d0 |
|
Just to take a step back, why is the "identity element" better for serialization? |
|
How do we store something like Currently all our plumbing is setup for a long vector of Float64 for serialized. |
|
PS, the majority of work for this is upgrade is in IIF, so i'm happy to do it in two iterations, but would just like to keep moving rather than grinding away in DFG too long |
src/services/DFGVariable.jl
Outdated
| function getPoint(::Type{T}, v::AbstractVector) where {T <: InferenceVariable} | ||
| M = getManifold(T) | ||
| p0 = getPointIdentity(T) | ||
| X = get_vector(M, p0, v, DefaultOrthonormalBasis()) | ||
| exp(M, p0, X) | ||
| end | ||
|
|
||
| """ | ||
| $SIGNATURES | ||
|
|
||
| Default reduction of a variable point value (a group element) into coordinates as `Vector`. Override if defaults are not correct. | ||
|
|
||
| Related | ||
|
|
||
| [`getPoint`](@ref) | ||
| """ | ||
| function getCoordinates(::Type{T}, p) where {T <: InferenceVariable} | ||
| M = getManifold(T) | ||
| p0 = getPointIdentity(T) | ||
| X = log(M, p0, p) | ||
| get_coordinates(M, p0, X, DefaultOrthonormalBasis()) | ||
| end |
There was a problem hiding this comment.
I'm not sure about this. It's not general and makes a lot of assumptions.
Perhaps it can be more accurate with a new abstract LieGroupInferenceVariable <: InferenceVariable (but also don't like it that much)
There was a problem hiding this comment.
Lets not pre-optimize. I'm trying to take a reasonable step towards Manifolds based on where we are today. Manifolds.get_coordinates already exists, so my feeling is we can leverage that as default and user must override for general cases.
For this step, I'm totally against building new abstracts. Maybe later once vnd.val::Vector{P} is already working fully downstream.
Maybe I'm missing something, is it not as simple as: julia> A = SA[1 2; 3 4]
2×2 SMatrix{2, 2, Int64, 4} with indices SOneTo(2)×SOneTo(2):
1 2
3 4
julia> v = vec(A)
4-element SVector{4, Int64} with indices SOneTo(4):
1
3
2
4
julia> B = typeof(A)(v)
2×2 SMatrix{2, 2, Int64, 4} with indices SOneTo(2)×SOneTo(2):
1 2
3 4 |
|
Not everything is We can change all this after the first iteration |
|
busy changing bw back to matrix.. will commit in a few minutes |
I'm all for getting the ball rolling. As long as it doesn't get baked in because of serialization. I would rather design how it would work best for numerics and then have serialization fit in with that. So I say go ahead, as you mentioned the hard part will be downstream and we need to get to it. |
|
If we can serialize:
Is that not enough? If a user wants some other type they can implement a new serialize function Edit: I took matrix out, I think StaticArrays should be enough, but Vector is easy to do. |
I agree with that, we will revisit #590 so we can be a bit more loose with serialization now. I want to include in this design criteria how easy it is for a newcomer to build new variables and factors.
Cool, lets tag downstream working, and then come back to DFG and do a deprecation cycle for cleanup on the Manifolds.jl upgrade.
Think we (our early users) are very likely to want matrices, so should include that.
Yup, 100% agree. One thing I've wanted to do for a long time is make variables and factors use a
I'd say we do that as a cleanup, keeping bits of JSON3, #590, etc in mind; but that is after this work to transition to Manifolds.jl proper, and also get JuliaRobotics/IncrementalInference.jl#1010 resolved first. |
|
xref #763 |
fyi @Affie , im changing @defVariable slightly to take the identity point for a variable rather than just a point type. We still get the same point type information using
typeof. Let me know if there is a problem.Issue is we need for serialization something like
getCoordinatesandgetPoint. For the default dispatch, im just pointing through to the basic Manifoldsget_coordinatesandget_vectorfunctions. Users doing something more fancy should override DFGsgetCoordinatesandgetPointfunctions if needed.