JuliaRobotics / DistributedFactorGraphs.jl

Abstraction layer for spanning factor graphs over various technologies
Apache License 2.0
21 stars 2 forks source link

Should we deprecate lsfWho in favor of just lsf #707

Closed Affie closed 1 month ago

Affie commented 3 years ago

_From PR discussion: https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/pull/705#discussion_r540034178_

new name is: 'lsfTypes(fg, LinearRelative)'

@dehann There is no function like that. Only lsfTypes(dfg) -> Return Vector{Symbol} of all unique factor types in factor graph.

Did you perhaps mean just lsf as the test just above that?

Should we deprecate lsfWho in favour of just lsf

In this case lsfWho and lsf have almost identical functionality with different implementations. I would say we keep on only lsf for example lsf(fg, LinearRelative)

It can work if there is not a need for lsf(fg, :LinearRelative)

dehann commented 3 years ago

yeah that's fine, agree with: don't pass a Symbol, just pass the type (especially to resolve dispatch). 'lsf(fg, Pose2Pose2)'

This function already exists somewhere because i use it quite often. Same goes for 'ls(fg, Pose3)'.

dehann commented 3 years ago

@Affie , i agree with your idea to just have simple ls and lsf dispatch. bi quickly tried and this is already working:

using RoME

fg = generateCanonicalFG_Hexagonal(graphinit=false);

julia> lsf(fg, Pose2Pose2)
6-element Array{Symbol,1}:
 :x3x4f1
 :x4x5f1
 :x5x6f1
 :x0x1f1
 :x1x2f1
 :x2x3f1

If you don't mind, please check if there are any other loose bits around. We definitely need to deprecate things like lsfWho.

Affie commented 2 months ago