Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Towards Singleton Softtypes and getDimension getManifolds #316

Merged
merged 2 commits into from
Aug 5, 2020

Conversation

Affie
Copy link
Member

@Affie Affie commented Aug 4, 2020

No description provided.

@@ -566,7 +566,7 @@ ls(sfg, :x19)

pts = approxConv(sfg, :x19l0f1, :x19)

X19 = manikde!(pts, Pose2().manifolds)
X19 = manikde!(pts, getManifolds(Pose2()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work manikde!(pts, Pose2()).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I don't know the API yet. I just used direct replacement. Should we change all of them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to change them, just wanted to share so that you are aware. The internal function are doing the same thing you are getManifolds(T())

@@ -11,3 +11,5 @@ mutable struct DynPoint2 <: IncrementalInference.InferenceVariable
manifolds::Tuple{Symbol, Symbol, Symbol, Symbol}
DynPoint2(;ut::Int64=-9999999999) = new(ut, 4,(:Euclid,:Euclid,:Euclid,:Euclid,))
end
getDimension(::DynPoint2) = 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can also consider avoiding the object allocation with getDimension(::Type{DynPoint2}) = 4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can consider that at a later stage. For now, the softtype field still exists for compatibility with types such as DynPose2, so instances are passed.

@dehann
Copy link
Member

dehann commented Aug 5, 2020

From Travis
https://travis-ci.org/github/JuliaRobotics/RoME.jl/jobs/715058286

Got exception outside of a @test

  type Pose3 has no field dims

  Stacktrace:

    [1] getproperty(x::Pose3, f::Symbol)

      @ Base ./Base.jl:33

    [2] addVariable!(dfg::LightDFG{SolverParams,DistributedFactorGraphs.DFGVariable,DistributedFactorGraphs.DFGFactor}, lbl::Symbol, softtype::Pose3; N::Int64, autoinit::Nothing, solvable::Int64, timestamp::TimeZones.ZonedDateTime, dontmargin::Bool, labels::Nothing, tags::Vector{Symbol}, smalldata::Dict{String,String}, checkduplicates::Bool, initsolvekeys::Vector{Symbol})

      @ IncrementalInference ~/.julia/packages/IncrementalInference/TFc0I/src/FactorGraph.jl:415

    [3] addVariable!(dfg::LightDFG{SolverParams,DistributedFactorGraphs.DFGVariable,DistributedFactorGraphs.DFGFactor}, lbl::Symbol, softtype::Type{Pose3}; N::Int64, autoinit::Nothing, timestamp::TimeZones.ZonedDateTime, solvable::Int64, dontmargin::Bool, labels::Nothing, tags::Vector{Symbol}, smalldata::Dict{String,String})

      @ IncrementalInference ~/.julia/packages/IncrementalInference/TFc0I/src/FactorGraph.jl:446

    [4] top-level scope

      @ ~/build/JuliaRobotics/RoME.jl/test/testPose3Pose3NH.jl:42

    [5] top-level scope

      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1114

    [6] top-level scope

      @ ~/build/JuliaRobotics/RoME.jl/test/testPose3Pose3NH.jl:42

    [7] include(fname::String)

      @ Base.MainInclude ./client.jl:443

    [8] top-level scope

      @ ~/build/JuliaRobotics/RoME.jl/test/runtests.jl:44

    [9] include(fname::String)

      @ Base.MainInclude ./client.jl:443

   [10] top-level scope

      @ none:6

   [11] eval(m::Module, e::Any)

      @ Core ./boot.jl:344

   [12] exec_options(opts::Base.JLOptions)

      @ Base ./client.jl:260

   [13] _start()

      @ Base ./client.jl:484

@Affie
Copy link
Member Author

Affie commented Aug 5, 2020

type Pose3 has no field dims

This is by design as part of the move to singleton softtypes. It needs DFG and IIF masters to pass.

@dehann
Copy link
Member

dehann commented Aug 5, 2020

gotcha, thanks!

@Affie Affie marked this pull request as ready for review August 5, 2020 11:50
@Affie Affie merged commit 7b2f3bb into master Aug 5, 2020
@Affie
Copy link
Member Author

Affie commented Aug 5, 2020

Test are passing on masters, so merging.

@Affie Affie deleted the maint/20Q3/softtype_getDimension_getManifolds branch February 1, 2021 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants