POC for SE and SO #45
Conversation
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
- Coverage 53.81% 46.41% -7.40%
==========================================
Files 2 3 +1
Lines 433 502 +69
==========================================
Hits 233 233
- Misses 200 269 +69
Continue to review full report at Codecov.
|
|
@Affie please see review notes and be advised i changed the merge base to master for this PR |
|
Note this PR is blocking RoME v0.15.2, see JuliaRobotics/RoME.jl#449 |
|
ah, it might show up on the "Files" view of the PR: |
It is still not showing up this side. |
| ##============================================================================== | ||
|
|
||
| abstract type AbstractLieGroup end | ||
| abstract type AbstractLieAlgebra end |
There was a problem hiding this comment.
Hi Johan, i'm not so sure about adding new abstracts here. See Manifolds docs:
https://juliamanifolds.github.io/Manifolds.jl/latest/manifolds/group.html#Group-manifolds-and-actions
could you avoid adding a new abstraction below Manifolds? If these new abstracts are just to reduce the function definitions, I think it would be better to write out the functions instead -- i.e. write against <: SE or <: se rather than new abstracts?
There was a problem hiding this comment.
If these new abstracts are just to reduce the function definitions, I think it would be better to write out the functions instead -- i.e. write against <: SE or <: se rather than new abstracts?
Yes, it is just to reduce function definitions.
AbstractLieGroup is not the correct name as it's a combination of a point and a manifold.
Maybe something like AbstractGroupManifoldPoint and AbstractGroupAlgebraVector is more accurate.
That is this wrapper exists, lets continue in JuliaRobotics/RoME.jl#450
| Base.convert(::Type{AbstractLieAlgebra}, ::Type{SO{N, T}}) where {N,T} = so{N} | ||
|
|
||
| ##============================================================================== | ||
| ## RoME like functions sandbox |
There was a problem hiding this comment.
we should move all these into RoME and replace the outdated RobotUtils.jl and OdometryUtils.jl currently in RoME
There was a problem hiding this comment.
see related functions being moved in JuliaRobotics/RoME.jl#449
There was a problem hiding this comment.
jip, just a sandbox to test the functions. not intended to stay.
|
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 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. which is easily overloaded for |
No description provided.