Merge pull request #466 from JuliaRobotics/maint/20q2/immutable_and_rem_nodeparams

Affie · web-flow · commit 18143914c361 · 2020-05-16T12:10:19.000+02:00
Remove DFGNodeParams and make DFGFactor immutable
diff --git a/src/Deprecated.jl b/src/Deprecated.jl
@@ -7,6 +7,8 @@
 ## Remove in 0.9
 ##==============================================================================
 
+setTimestamp!(f::FactorDataLevel1, ts::DateTime) = error("setTimestamp!(f::FactorDataLevel1, ts::DateTime) is deprecated")
+
 include("../attic/GraphsDFG/GraphsDFG.jl")
 @reexport using .GraphsDFGs
 
diff --git a/src/DistributedFactorGraphs.jl b/src/DistributedFactorGraphs.jl
@@ -90,7 +90,6 @@ export DFGVariable, DFGVariableSummary, SkeletonDFGVariable
 export DFGFactor, DFGFactorSummary, SkeletonDFGFactor
 
 # Common
-export DFGNodeParams
 export getSolvable, setSolvable!, isSolvable
 export getInternalId
 
diff --git a/src/entities/AbstractDFG.jl b/src/entities/AbstractDFG.jl
@@ -20,13 +20,6 @@ An abstract DFG factor.
 abstract type AbstractDFGFactor <: DFGNode
 end
 
-"""
-$(TYPEDEF)
-The common node parameters for variables and factors.
-"""
-mutable struct DFGNodeParams
-    solvable::Int
-end
 
 """
 $(TYPEDEF)
diff --git a/src/entities/DFGFactor.jl b/src/entities/DFGFactor.jl
@@ -94,7 +94,7 @@ Complete factor structure for a DistributedFactorGraph factor.
 Fields:
 $(TYPEDFIELDS)
 """
-mutable struct DFGFactor{T} <: AbstractDFGFactor
+struct DFGFactor{T, N} <: AbstractDFGFactor
     """Factor label, e.g. :x1f1.
     Accessor: [`getLabel`](@ref)"""
     label::Symbol
@@ -106,17 +106,23 @@ mutable struct DFGFactor{T} <: AbstractDFGFactor
     tags::Set{Symbol}
     """Solver data.
     Accessors: [`getSolverData`](@ref), [`setSolverData!`](@ref)"""
-    solverData::GenericFunctionNodeData{T}
+    solverData::Base.RefValue{GenericFunctionNodeData{T}}
     """Solvable flag for the factor.
-    Accessors: [`getSolvable`](@ref), [`setSolvable!`](@ref)
-    TODO: Switch to `DFGNodeParams`"""
-    solvable::Int #TODO we can go with DFGNodeParams or Reference the solvable field with getproperty overload
-    """Mutable parameters for the variable. We suggest using accessors to get to this data.
     Accessors: [`getSolvable`](@ref), [`setSolvable!`](@ref)"""
-    _dfgNodeParams::DFGNodeParams
+    solvable::Base.RefValue{Int}
     """Internal cache of the ordering of the neighbor variables. Rather use getVariableOrder to get the list as this is an internal value.
     Accessors: [`getVariableOrder`](@ref)"""
-    _variableOrderSymbols::Vector{Symbol}
+    _variableOrderSymbols::NTuple{N,Symbol}
+
+    # Inner constructor
+    DFGFactor{T}(label::Symbol,
+                 timestamp::DateTime,
+                 tags::Set{Symbol},
+                 solverData::GenericFunctionNodeData{T},
+                 solvable::Int,
+                 _variableOrderSymbols::NTuple{N,Symbol}) where {T,N} =
+                 new{T,N}(label, timestamp, tags, Ref(solverData), Ref(solvable), _variableOrderSymbols)
+
 end
 
 ##------------------------------------------------------------------------------
@@ -127,8 +133,17 @@ $(SIGNATURES)
 
 Construct a DFG factor given a label.
 """
-DFGFactor{T}(label::Symbol, timestamp::DateTime=now()) where {T} =
-                DFGFactor(label, timestamp, Set{Symbol}(), GenericFunctionNodeData{T}(), 1, DFGNodeParams(1), Symbol[])
+DFGFactor(label::Symbol,
+          timestamp::DateTime,
+          tags::Set{Symbol},
+          solverData::GenericFunctionNodeData{T},
+          solvable::Int,
+          _variableOrderSymbols::Tuple) where {T} =
+          DFGFactor{T}(label, timestamp, tags, solverData, solvable, _variableOrderSymbols)
+
+
+DFGFactor{T}(label::Symbol, variableOrderSymbols::Vector{Symbol}, timestamp::DateTime=now(), data::GenericFunctionNodeData{T} = GenericFunctionNodeData{T}()) where {T} =
+                DFGFactor(label, timestamp, Set{Symbol}(), data, 1, Tuple(variableOrderSymbols))
 
 
 DFGFactor(label::Symbol,
@@ -137,22 +152,23 @@ DFGFactor(label::Symbol,
           tags::Set{Symbol}=Set{Symbol}(),
           timestamp::DateTime=now(),
           solvable::Int=1) where {T} =
-                DFGFactor{T}(label,timestamp,tags,data,solvable,DFGNodeParams(solvable),variableOrderSymbols)
+                DFGFactor{T}(label, timestamp, tags, data, solvable, Tuple(variableOrderSymbols))
 
 
 
 Base.getproperty(x::DFGFactor,f::Symbol) = begin
-    if f == :solvable
-        getfield(x,:_dfgNodeParams).solvable
+    if f == :solvable || f == :solverData
+        getfield(x, f)[]
+    elseif f == :_variableOrderSymbols
+        [getfield(x,f)...]
     else
         getfield(x,f)
     end
 end
 
 Base.setproperty!(x::DFGFactor,f::Symbol, val) = begin
-    if f == :solvable
-        setfield!(x,f,val)
-        getfield(x,:_dfgNodeParams).solvable = val
+    if f == :solvable || f == :solverData
+        getfield(x,f)[] = val
     else
         setfield!(x,f,val)
     end
diff --git a/src/entities/DFGVariable.jl b/src/entities/DFGVariable.jl
@@ -200,9 +200,9 @@ struct DFGVariable{T<:InferenceVariable} <: AbstractDFGVariable
     """Dictionary of large data associated with this variable.
     Accessors: [`addBigDataEntry!`](@ref), [`getBigDataEntry`](@ref), [`updateBigDataEntry!`](@ref), and [`deleteBigDataEntry!`](@ref)"""
     bigData::Dict{Symbol, AbstractBigDataEntry}
-    """Mutable parameters for the variable. We suggest using accessors to get to this data.
+    """Solvable flag for the variable.
     Accessors: [`getSolvable`](@ref), [`setSolvable!`](@ref)"""
-    _dfgNodeParams::DFGNodeParams
+    solvable::Base.RefValue{Int}
 end
 
 ##------------------------------------------------------------------------------
@@ -220,7 +220,7 @@ DFGVariable(label::Symbol, softtype::T;
             smallData::Dict{String, String}=Dict{String, String}(),
             bigData::Dict{Symbol, AbstractBigDataEntry}=Dict{Symbol,AbstractBigDataEntry}(),
             solvable::Int=1) where {T <: InferenceVariable} =
-    DFGVariable{T}(label, timestamp, tags, estimateDict, solverDataDict, smallData, bigData, DFGNodeParams(solvable))
+    DFGVariable{T}(label, timestamp, tags, estimateDict, solverDataDict, smallData, bigData, Ref(solvable))
 
 
 DFGVariable(label::Symbol,
@@ -231,19 +231,19 @@ DFGVariable(label::Symbol,
             smallData::Dict{String, String}=Dict{String, String}(),
             bigData::Dict{Symbol, AbstractBigDataEntry}=Dict{Symbol,AbstractBigDataEntry}(),
             solvable::Int=1) where {T <: InferenceVariable} =
-    DFGVariable{T}(label, timestamp, tags, estimateDict, Dict{Symbol, VariableNodeData{T}}(:default=>solverData), smallData, bigData, DFGNodeParams(solvable))
+    DFGVariable{T}(label, timestamp, tags, estimateDict, Dict{Symbol, VariableNodeData{T}}(:default=>solverData), smallData, bigData, Ref(solvable))
 
 Base.getproperty(x::DFGVariable,f::Symbol) = begin
     if f == :solvable
-        getfield(x,:_dfgNodeParams).solvable
+        getfield(x,f)[]
     else
         getfield(x,f)
     end
 end
 
 Base.setproperty!(x::DFGVariable,f::Symbol, val) = begin
     if f == :solvable
-        getfield(x,:_dfgNodeParams).solvable = val
+        getfield(x,f)[] = val
     else
         setfield!(x,f,val)
     end
diff --git a/src/services/AbstractDFG.jl b/src/services/AbstractDFG.jl
@@ -379,12 +379,18 @@ $(SIGNATURES)
 """
 function addFactor!(dfg::AbstractDFG, variables::Vector{<:AbstractDFGVariable}, factor::F)::F where  F <: AbstractDFGFactor
 
-        variableLabels = map(v->v.label, variables)
+    Base.depwarn("addFactor!(dfg, variables, factor) is deprecated, use addFactor!(dfg, factor)", :addFactor!)
+    variableLabels = map(v->v.label, variables)
 
+    if factor isa DFGFactor
+        f = factor
+        newfactor =  DFGFactor(f.label, f.timestamp, f.tags, f.solverData, f.solvable, Tuple(variableLabels))
+        return addFactor!(dfg, newfactor)
+    else
         resize!(factor._variableOrderSymbols, length(variableLabels))
         factor._variableOrderSymbols .= variableLabels
-
         return addFactor!(dfg, factor)
+    end
 
 end
 
@@ -393,10 +399,18 @@ $(SIGNATURES)
 """
 function addFactor!(dfg::AbstractDFG, variableLabels::Vector{Symbol}, factor::F)::AbstractDFGFactor where F <: AbstractDFGFactor
 
-    resize!(factor._variableOrderSymbols, length(variableLabels))
-    factor._variableOrderSymbols .= variableLabels
+    Base.depwarn("addFactor!(dfg, variables, factor) is deprecated, use addFactor!(dfg, factor)", :addFactor!)
+
+    if factor isa DFGFactor
+        f = factor
+        newfactor =  DFGFactor(f.label, f.timestamp, f.tags, f.solverData, f.solvable, Tuple(variableLabels))
+        return addFactor!(dfg, newfactor)
+    else
+        resize!(factor._variableOrderSymbols, length(variableLabels))
+        factor._variableOrderSymbols .= variableLabels
+        return addFactor!(dfg, factor)
+    end
 
-    return addFactor!(dfg, factor)
 end
 
 
diff --git a/src/services/CommonAccessors.jl b/src/services/CommonAccessors.jl
@@ -84,7 +84,7 @@ Variables or factors may or may not be 'solvable', depending on a user definitio
 Related:
 - isSolveInProgress
 """
-getSolvable(var::Union{DFGVariable, DFGFactor})::Int = var._dfgNodeParams.solvable
+getSolvable(var::Union{DFGVariable, DFGFactor})::Int = var.solvable
 #TODO DataLevel2
 
 """
@@ -94,9 +94,9 @@ Get 'solvable' parameter for either a variable or factor.
 """
 function getSolvable(dfg::AbstractDFG, sym::Symbol)::Int
   if isVariable(dfg, sym)
-    return getVariable(dfg, sym)._dfgNodeParams.solvable
+    return getVariable(dfg, sym).solvable
   elseif isFactor(dfg, sym)
-    return getFactor(dfg, sym)._dfgNodeParams.solvable
+    return getFactor(dfg, sym).solvable
   end
 end
 
@@ -107,7 +107,7 @@ end
 Set the `solvable` parameter for either a variable or factor.
 """
 function setSolvable!(node::N, solvable::Int)::Int where N <: DFGNode
-  node._dfgNodeParams.solvable = solvable
+  node.solvable = solvable
   return solvable
 end
 
@@ -119,9 +119,9 @@ Set the `solvable` parameter for either a variable or factor.
 """
 function setSolvable!(dfg::AbstractDFG, sym::Symbol, solvable::Int)::Int
   if isVariable(dfg, sym)
-    getVariable(dfg, sym)._dfgNodeParams.solvable = solvable
+    getVariable(dfg, sym).solvable = solvable
   elseif isFactor(dfg, sym)
-    getFactor(dfg, sym)._dfgNodeParams.solvable = solvable
+    getFactor(dfg, sym).solvable = solvable
   end
   return solvable
 end
diff --git a/src/services/CompareUtils.jl b/src/services/CompareUtils.jl
@@ -18,7 +18,7 @@ implement compare if needed.
 
 
 # Generate compares automatically for all in this union
-const GeneratedCompareUnion = Union{MeanMaxPPE, VariableNodeData, DFGNodeParams,
+const GeneratedCompareUnion = Union{MeanMaxPPE, VariableNodeData,
                               DFGVariable, DFGVariableSummary, SkeletonDFGVariable,
                               GenericFunctionNodeData,
                               DFGFactor, DFGFactorSummary, SkeletonDFGFactor}
diff --git a/src/services/CustomPrinting.jl b/src/services/CustomPrinting.jl
@@ -52,7 +52,7 @@ function printVariable(io::IO, vert::DFGVariable;
 
         for f in fields
             printstyled(ioc, f,":\n", color=:blue)
-            show(ioc, getfield(vert, f))
+            show(ioc, getproperty(vert, f))
             println(ioc)
         end
     end
@@ -86,7 +86,7 @@ function printFactor(io::IO, vert::DFGFactor;
 
         for f in fields
             printstyled(ioc, f,":\n", color=:blue)
-            show(ioc, getfield(vert, f))
+            show(ioc, getproperty(vert, f))
             println(ioc)
         end
     end
diff --git a/src/services/DFGFactor.jl b/src/services/DFGFactor.jl
@@ -69,15 +69,13 @@ end
 
 
 function setTimestamp(f::DFGFactor, ts::DateTime)
-    return DFGFactor(f.label, ts, f.tags, f.solverData, f.solvable, f._dfgNodeParams, f._variableOrderSymbols)
+    return DFGFactor(f.label, ts, f.tags, f.solverData, f.solvable, getfield(f,:_variableOrderSymbols))
 end
 
 function setTimestamp(f::DFGFactorSummary, ts::DateTime)
     return DFGFactorSummary(f.label, ts, f.tags, f._variableOrderSymbols)
 end
 
-setTimestamp!(f::FactorDataLevel1, ts::DateTime) = f.timestamp = ts
-
 
 ##------------------------------------------------------------------------------
 ## solvable
@@ -89,7 +87,7 @@ setTimestamp!(f::FactorDataLevel1, ts::DateTime) = f.timestamp = ts
 # isSolvable
 
 ##------------------------------------------------------------------------------
-## _dfgNodeParams [solvable]
+## solvable
 ##------------------------------------------------------------------------------
 
 ## COMMON
@@ -122,7 +120,6 @@ function getSolverData(f::F) where F <: DFGFactor
   return f.solverData
 end
 
-#TODO don't know if this is used, added for completeness
 setSolverData!(f::DFGFactor, data::GenericFunctionNodeData) = f.solverData = data
 
 
diff --git a/src/services/DFGVariable.jl b/src/services/DFGVariable.jl
@@ -166,7 +166,7 @@ function setTimestamp(v::DFGVariable, ts::DateTime; verbose::Bool=true)
     if verbose
         @warn "verbose=true: setTimestamp(::DFGVariable,...) creates a returns a new immutable DFGVariable object (and didn't change a distributed factor graph object), make sure you are using the right pointers: getVariable(...).  See setTimestamp!(...) and note suggested use is at addVariable!(..., [timestamp=...]).  See DFG #315 for explanation."
     end
-    return DFGVariable(v.label, ts, v.tags, v.ppeDict, v.solverDataDict, v.smallData, v.bigData, v._dfgNodeParams)
+    return DFGVariable(v.label, ts, v.tags, v.ppeDict, v.solverDataDict, v.smallData, v.bigData, Ref(v.solvable))
 end
 
 function setTimestamp(v::DFGVariableSummary, ts::DateTime; verbose::Bool=true)
@@ -178,7 +178,7 @@ end
 
 
 ##------------------------------------------------------------------------------
-## _dfgNodeParams [solvable]
+## solvable
 ##------------------------------------------------------------------------------
 
 ## COMMON: solvable
diff --git a/src/services/Serialization.jl b/src/services/Serialization.jl
@@ -37,7 +37,7 @@ function unpackVariable(dfg::G, packedProps::Dict{String, Any})::DFGVariable whe
     solverData = Dict(Symbol.(keys(packed)) .=> map(p -> unpackVariableNodeData(dfg, p), values(packed)))
 
     # Rebuild DFGVariable using the first solver softtype in solverData
-    variable = DFGVariable{softtype}(Symbol(packedProps["label"]), timestamp, Set(tags), ppeDict, solverData,  smallData, Dict{Symbol,AbstractBigDataEntry}(), DFGNodeParams(packedProps["solvable"]))
+    variable = DFGVariable{softtype}(Symbol(packedProps["label"]), timestamp, Set(tags), ppeDict, solverData,  smallData, Dict{Symbol,AbstractBigDataEntry}(), Ref(packedProps["solvable"]))
 
     # Now rehydrate complete bigData type.
     for (k,bdeInter) in bigDataIntermed
@@ -159,13 +159,13 @@ function unpackFactor(dfg::G, packedProps::Dict{String, Any})::DFGFactor where G
 
     # Rebuild DFGFactor
     #TODO use constuctor to create factor
-    factor = DFGFactor{typeof(fullFactorData.fnc)}(Symbol(label), timestamp)
+    factor = DFGFactor(Symbol(label),
+                       timestamp,
+                       Set(tags),
+                       fullFactorData,
+                       solvable,
+                       Tuple(_variableOrderSymbols))
 
-    union!(factor.tags, tags)
-    # factor.data = fullFactorData #TODO
-    setSolverData!(factor, fullFactorData)
-    factor._variableOrderSymbols = _variableOrderSymbols
-    setSolvable!(factor, solvable)
 
     # Note, once inserted, you still need to call rebuildFactorMetadata!
     return factor
diff --git a/test/DataStoreTests.jl b/test/DataStoreTests.jl
@@ -8,15 +8,15 @@ dfg = LightDFG{NoSolverParams}()
 struct TestInferenceVariable1 <: InferenceVariable end
 v1 = DFGVariable(:a, TestInferenceVariable1())
 v2 = DFGVariable(:b, TestInferenceVariable1())
-f1 = DFGFactor{TestFunctorInferenceType1}(:f1)
+f1 = DFGFactor{TestFunctorInferenceType1}(:f1, [:a,:b])
 #add tags for filters
 union!(v1.tags, [:VARIABLE, :POSE])
 union!(v2.tags, [:VARIABLE, :LANDMARK])
 union!(f1.tags, [:FACTOR])
 # @testset "Creating Graphs" begin
 addVariable!(dfg, v1)
 addVariable!(dfg, v2)
-addFactor!(dfg, [v1, v2], f1)
+addFactor!(dfg, f1)
 
 # Stores to test
 testStores = [InMemoryDataStore(), FileDataStore("/tmp/dfgFilestore")]
diff --git a/test/interfaceTests.jl b/test/interfaceTests.jl
@@ -36,7 +36,7 @@ end
     @test printFactor(fac1) == nothing
 
     @test printVariable(iobuf, var1, skipfields=[:timestamp, :solver, :ppe]) == nothing
-    @test String(take!(iobuf)) == "DFGVariable{TestSofttype1}\nlabel:\n:a\ntags:\nSet([:VARIABLE, :POSE])\nsmallData:\nDict(\"small\"=>\"data\")\nbigData:\nDict{Symbol,AbstractBigDataEntry}()\n_dfgNodeParams:\nDFGNodeParams(0)\n"
+    @test String(take!(iobuf)) == "DFGVariable{TestSofttype1}\nlabel:\n:a\ntags:\nSet([:VARIABLE, :POSE])\nsmallData:\nDict(\"small\"=>\"data\")\nbigData:\nDict{Symbol,AbstractBigDataEntry}()\nsolvable:\n0\n"
 
     @test printVariable(iobuf, var1, short=true) == nothing
     @test String(take!(iobuf)) == "DFGVariable{TestSofttype1}\nlabel: a\ntags: Set([:VARIABLE, :POSE])\nsize marginal samples: (1, 1)\nkde bandwidths: [0.0]\nNo PPEs\n"
@@ -45,7 +45,7 @@ end
     @test printFactor(iobuf, fac1, skipfields=[:timestamp, :solver]) == nothing
     @test occursin(r"DFGFactor.*\nlabel:\n:abf1", String(take!(iobuf)))
 
-    String(take!(iobuf)) == "DFGFactor{TestCCW{TestFunctorInferenceType1}}\nlabel:\n:abf1\ntags:\nSet([:tag1, :tag2])\nsolvable:\n0\n_dfgNodeParams:\nDFGNodeParams(1)\n_variableOrderSymbols:\n[:a, :b]\n"
+    String(take!(iobuf)) == "DFGFactor{TestCCW{TestFunctorInferenceType1}}\nlabel:\n:abf1\ntags:\nSet([:tag1, :tag2])\nsolvable:\n0\nsolvable:\n1\n_variableOrderSymbols:\n[:a, :b]\n"
 
     @test printFactor(iobuf, fac1, short=true) == nothing
     @test occursin(r"DFGFactor.*\nlabel.*\ntimestamp.*\ntags.*\nsolvable", String(take!(iobuf)))
diff --git a/test/plottingTest.jl b/test/plottingTest.jl
diff --git a/test/testBlocks.jl b/test/testBlocks.jl
