Changes from worksession

Co-authored-by: Dehann Fourie <dehann@users.noreply.github.com>

Author: Affie

src/Deprecated.jl

@@ -119,11 +119,17 @@ function solveGraphParametric2(
   return d, result, flatvar.idx, Σ
 end
 
+##==============================================================================
+## Deprecate code below before v0.35
+##==============================================================================
+
+@deprecate _prepCCW(w...;kw...) _createCCW(w...;kw...)
 
 ##==============================================================================
 ## Deprecate code below before v0.34
 ##==============================================================================
 
+
 # function CommonConvWrapper(
 #   usrfnc::T,
 #   fullvariables, #::Tuple ::Vector{<:DFGVariable};

src/Serialization/services/DispatchPackedConversions.jl

@@ -37,12 +37,12 @@ function reconstFactorData(
   vars = map(f -> getVariable(dfg, f), varOrder)
   userCache = preambleCache(dfg, vars, usrfnc)
 
-  # TODO -- improve _prepCCW for hypotheses and certainhypo field recovery when deserializing
+  # TODO -- improve _createCCW for hypotheses and certainhypo field recovery when deserializing
   # reconstitute from stored data
   # FIXME, add threadmodel=threadmodel
   # FIXME https://github.com/JuliaRobotics/DistributedFactorGraphs.jl/issues/590#issuecomment-776838053
   # FIXME dont know what manifolds to use in ccw
-  ccw = _prepCCW(
+  ccw = _createCCW(
     vars,
     usrfnc;
     multihypo,
@@ -145,7 +145,7 @@ function rebuildFactorMetadata!(
   end
 
   #... Copying neighbor data into the factor?
-  # JT TODO it looks like this is already updated in getDefaultFactorData -> _prepCCW
+  # JT TODO it looks like this is already updated in getDefaultFactorData -> _createCCW
   # factormetadata.variableuserdata is deprecated, remove when removing deprecation
   # for i in 1:Threads.nthreads()
   #   ccw_new.fnc.cpt[i].factormetadata.variableuserdata = deepcopy(neighborUserData)

src/entities/FactorOperationalMemory.jl

@@ -83,7 +83,7 @@ Related
 Base.@kwdef struct CommonConvWrapper{
   T <: AbstractFactor, 
   VT <: Tuple,
-  NTP <: Tuple, 
+  TP <: Tuple, 
   CT,
   AM <: AbstractManifold,
   HP <: Union{Nothing, <:Distributions.Categorical{Float64, Vector{Float64}}},
@@ -98,8 +98,9 @@ Base.@kwdef struct CommonConvWrapper{
   fullvariables::VT
   # shortcuts to numerical containers
   """ Numerical containers for all connected variables.  Hypo selection needs to be passed 
-      to each hypothesis evaluation event on user function via CalcFactor, #1321 """
-  varValsAll::NTP
+      to each hypothesis evaluation event on user function via CalcFactor, #1321.
+      Points directly at the variable VND.val (not a deepcopy). """
+  varValsAll::TP
   """ dummy cache value to be deep copied later for each of the CalcFactor instances """
   dummyCache::CT = nothing
   # derived config parameters for this factor

src/services/ApproxConv.jl

@@ -12,23 +12,36 @@ function approxConvBelief(
   skipSolve::Bool = false,
 )
   #
-  v1 = getVariable(dfg, target)
-  N = N == 0 ? getNumPts(v1; solveKey = solveKey) : N
+  v_trg = getVariable(dfg, target)
+  N = N == 0 ? getNumPts(v_trg; solveKey = solveKey) : N
+  # approxConv should push its result into duplicate memory destination, NOT the variable.VND.val itself.  ccw.varValsAll always points directly to variable.VND.val
   # points and infoPerCoord
-  pts, ipc = evalFactor(dfg, fc, v1.label, measurement; solveKey, N, skipSolve, nullSurplus)
+
+  pts, ipc = evalFactor(
+    dfg, 
+    fc, 
+    v_trg.label, 
+    measurement; 
+    solveKey, 
+    N, 
+    skipSolve, 
+    nullSurplus
+  )
 
   len = length(ipc)
   mask = 1e-14 .< abs.(ipc)
   partl = collect(1:len)[mask]
 
   # is the convolution infoPerCoord full or partial
-  if sum(mask) == len
+  res = if sum(mask) == len
     # not partial
-    return manikde!(getManifold(getVariable(dfg, target)), pts; partial = nothing)
+    manikde!(getManifold(getVariable(dfg, target)), pts; partial = nothing)
   else
     # is partial
-    return manikde!(getManifold(getVariable(dfg, target)), pts; partial = partl)
+    manikde!(getManifold(getVariable(dfg, target)), pts; partial = partl)
   end
+    
+  return res
 end
 
 approxConv(w...; kw...) = getPoints(approxConvBelief(w...; kw...), false)
@@ -96,7 +109,9 @@ function approxConvBelief(
     neMsk = exists.(tfg, varLbls) .|> x -> xor(x, true)
     # put the non-existing variables into the temporary graph `tfg`
     # bring all the solveKeys too
-    addVariable!.(tfg, getVariable.(dfg, varLbls[neMsk]))
+    for v in getVariable.(dfg, varLbls[neMsk])
+      addVariable!(tfg, v.label, getVariableType(v))
+    end
     # variables adjacent to the shortest path should be initialized from dfg
     setdiff(varLbls, path[xor.(fctMsk, true)]) .|>
     x -> initVariable!(tfg, x, getBelief(dfg, x))

src/services/CalcFactor.jl

@@ -220,58 +220,42 @@ Function returns with ARR[sfidx] pointing at newly allocated deepcopy of the
 existing values in getVal(Xi[.label==solvefor]).
 
 Notes
+- 2023Q2 intended use, only create VarValsAll the first time a factor added/reconstructed
 - Return values `sfidx` is the element in ARR where `Xi.label==solvefor` and
 - `maxlen` is length of all (possibly resampled) `ARR` contained particles.
 - `Xi` is order sensitive.
 - for initialization, solveFor = Nothing.
 - `P = getPointType(<:InferenceVariable)`
-
-DevNotes
-- FIXME ARR internally should become a NamedTuple
 """
-function _prepParamVec(
-  Xi::Vector{<:DFGVariable},
-  solvefor::Union{Nothing, Symbol},
-  N::Int = 0;
+function _createVarValsAll(
+  Xi::Vector{<:DFGVariable};
   solveKey::Symbol = :default,
 )
   #
-  # FIXME refactor to new NamedTuple instead
-  varParamsAll = getVal.(Xi; solveKey)
-  # Xi_labels = getLabel.(Xi)
-  sfidx = if isnothing(solvefor)
-    0
-  else
-    findfirst(==(solvefor), getLabel.(Xi))
-  end
-  # sfidx = isnothing(sfidx) ? 0 : sfidx
+  # Note, NamedTuple once upon a time created way too much recompile load on repeat solves, #1564
+  varValsAll = map(var_i->getVal(var_i; solveKey), tuple(Xi...))
 
-  # this line does nothing...
-  # maxlen = N # FIXME see #105
-
-  LEN = length.(varParamsAll)
-  maxlen = maximum([N; LEN])
+  # how many points
+  LEN = length.(varValsAll)
+  maxlen = maximum(LEN)
+  # NOTE, forcing maxlen to N results in errors (see test/testVariousNSolveSize.jl) see #105
+  # maxlen = N == 0 ? maxlen : N
 
+  # allow each variable to have a different number of points, which is resized during compute here
   # resample variables with too few kernels (manifolds points)
   SAMP = LEN .< maxlen
   for i = 1:length(Xi)
     if SAMP[i]
       Pr = getBelief(Xi[i], solveKey)
-      _resizePointsVector!(varParamsAll[i], Pr, maxlen)
+      _resizePointsVector!(varValsAll[i], Pr, maxlen)
     end
   end
 
   # TODO --rather define reusable memory for the proposal
   # we are generating a proposal distribution, not direct replacement for existing memory and hence the deepcopy.
-  if sfidx > 0
-    # FIXME, POSSIBLE SOURCE OF HUGE MEMORY CONSUMPTION ALLOCATION
-    varParamsAll[sfidx] = deepcopy(varParamsAll[sfidx])
-  end
+  #  POSSIBLE SOURCE OF HUGE MEMORY CONSUMPTION ALLOCATION
 
-  varValsAll = tuple(varParamsAll...)
-  # FIXME, forcing maxlen to N results in errors (see test/testVariousNSolveSize.jl) see #105
-  # maxlen = N == 0 ? maxlen : N
-  return varValsAll, maxlen, sfidx
+  return varValsAll
 end
 
 """
@@ -352,10 +336,12 @@ end
     $SIGNATURES
 
 Notes
+- _createCCW is likely only used when adding or reconstructing a new factor in the graph,
+  - else use _updateCCW 
 - Can be called with `length(Xi)==0`
 """
-function _prepCCW(
-  Xi::Vector{<:DFGVariable},
+function _createCCW(
+  Xi::AbstractVector{<:DFGVariable},
   usrfnc::T;
   multihypo::Union{Nothing, <:Distributions.Categorical} = nothing,
   nullhypo::Real = 0.0,
@@ -378,7 +364,8 @@ function _prepCCW(
   end
 
   # TODO check no Anys, see #1321
-  _varValsAll, maxlen, sfidx = _prepParamVec(Xi, nothing, 0; solveKey) # Nothing for init.
+  # NOTE, _varValsAll is only a reference to the actual VND.val memory of each variable
+  _varValsAll = _createVarValsAll(Xi; solveKey)
 
   manifold = getManifold(usrfnc)
   # standard factor metadata
@@ -435,7 +422,7 @@ function _prepCCW(
   pttypes = getVariableType.(Xi) .|> getPointType
   PointType = 0 < length(pttypes) ? pttypes[1] : Vector{Float64}
   if !isconcretetype(PointType)
-    @warn "_prepCCW PointType is not concrete $PointType" maxlog=50
+    @warn "_createCCW PointType is not concrete $PointType" maxlog=50
   end
 
   # PointType[],
@@ -487,40 +474,43 @@ approximate convolution computations.
 DevNotes
 - TODO consolidate with others, see https://github.com/JuliaRobotics/IncrementalInference.jl/projects/6
 """
-function _updateCCW!(
+function _beforeSolveCCW!(
   F_::Type{<:AbstractRelative},
   ccwl::CommonConvWrapper{F},
-  Xi::AbstractVector{<:DFGVariable},
-  solvefor::Symbol,
+  variables::AbstractVector{<:DFGVariable},
+  # solvefor::Symbol,
   N::Integer;
   measurement = Vector{Tuple{}}(),
   needFreshMeasurements::Bool = true,
   solveKey::Symbol = :default,
 ) where {F <: AbstractFactor} # F might be Mixture
   #
-  if length(Xi) !== 0
+  if length(variables) !== 0
     nothing
   else
-    @debug("cannot prep ccw.param list with length(Xi)==0, see DFG #590")
+    @debug("cannot prep ccw.param list with length(variables)==0, see DFG #590")
   end
 
-  # FIXME, order of fmd ccwl cf are a little weird and should be revised.
-  # FIXME maxlen should parrot N (barring multi-/nullhypo issues)
-  _varValsAll, maxlen, sfidx = _prepParamVec(Xi, solvefor, N; solveKey)
+  # TBD, order of fmd ccwl cf are a little weird and should be revised.
+  # TODO, maxlen should parrot N (barring multi-/nullhypo issues)
+  # set the 'solvefor' variable index -- i.e. which connected variable of the factor is being computed in this convolution. 
+  # ccwl.varidx[] = findfirst(==(solvefor), getLabel.(variables))
+  # everybody use maxlen number of points in belief function estimation
+  maxlen = maximum((N, length.(ccwl.varValsAll)...,))
 
-  # TODO, ensure all values (not just multihypothesis) is correctly used from here
-  for (i,varVal) in enumerate(_varValsAll)
-    resize!(ccwl.varValsAll[i],length(varVal))
-    ccwl.varValsAll[i][:] = varVal
-  end
+  ## PLAN B, make deep copy of ccwl.varValsAll[ccwl.varidx[]] just before the numerical solve 
 
-  # set the 'solvefor' variable index -- i.e. which connected variable of the factor is being computed in this convolution. 
-  ccwl.varidx[] = sfidx
+  # maxlen, ccwl.varidx[] = _updateParamVec(variables, solvefor, ccwl.varValsAll, N; solveKey)
+  # # TODO, ensure all values (not just multihypothesis) is correctly used from here
+  # for (i,varVal) in enumerate(_varValsAll)
+  #   resize!(ccwl.varValsAll[i],length(varVal))
+  #   ccwl.varValsAll[i][:] = varVal #TODO Kyk hierna: this looks like it will effectively result in vnd.val memory being overwritten 
+  # end
 
   # TODO better consolidation still possible
   # FIXME ON FIRE, what happens if this is a partial dimension factor?  See #1246
-  # FIXME, confirm this is hypo sensitive selection from Xi, better to use double indexing for clarity getDimension(ccw.fullvariables[hypoidx[sfidx]])
-  xDim = getDimension(getVariableType(Xi[sfidx])) # ccwl.varidx[]
+  # FIXME, confirm this is hypo sensitive selection from variables, better to use double indevariablesng for clarity getDimension(ccw.fullvariables[hypoidx[ccwl.varidx[]]])
+  xDim = getDimension(getVariableType(variables[ccwl.varidx[]])) # ccwl.varidx[]
   # ccwl.xDim = xDim
   # TODO maybe refactor different type or api call?
 
@@ -540,23 +530,25 @@ function _updateCCW!(
   # calculate new gradients
   # J = ccwl.gradients(measurement..., pts...)
 
-  return sfidx, maxlen
+  return maxlen
 end
 
-function _updateCCW!(
+function _beforeSolveCCW!(
   F_::Type{<:AbstractPrior},
   ccwl::CommonConvWrapper{F},
   Xi::AbstractVector{<:DFGVariable},
-  solvefor::Symbol,
+  # solvefor::Symbol,
   N::Integer;
   measurement = Vector{Tuple{}}(),
   needFreshMeasurements::Bool = true,
   solveKey::Symbol = :default,
 ) where {F <: AbstractFactor} # F might be Mixture
   # FIXME, NEEDS TO BE CLEANED UP AND WORK ON MANIFOLDS PROPER
   # fnc = ccwl.usrfnc!
-  sfidx = findfirst(getLabel.(Xi) .== solvefor)
-  @assert sfidx == 1 "Solving on Prior with CCW should have sfidx=1, priors are unary factors."
+  # sfidx = findfirst(getLabel.(Xi) .== solvefor)
+  @assert ccwl.varidx[] == 1 "Solving on Prior with CCW should have sfidx=1, priors are unary factors."
+  # @assert sfidx == 1 "Solving on Prior with CCW should have sfidx=1, priors are unary factors."
+  # ccwl.varidx[] = sfidx
   # sfidx = 1 #  why hardcoded to 1, maybe for only the AbstractPrior case here
 
   # setup the partial or complete decision variable dimensions for this ccwl object
@@ -570,30 +562,30 @@ function _updateCCW!(
   # update ccwl.measurement values
   updateMeasurement!(ccwl, maxlen; needFreshMeasurements, measurement, _allowThreads=true)
 
-  return sfidx, maxlen
+  return maxlen
 end
 
 # TODO, can likely deprecate this
-function _updateCCW!(
+function _beforeSolveCCW!(
   ccwl::Union{CommonConvWrapper{F}, CommonConvWrapper{Mixture{N_, F, S, T}}},
   Xi::AbstractVector{<:DFGVariable},
-  solvefor::Symbol,
+  # solvefor::Symbol,
   N::Integer;
   kw...,
 ) where {N_, F <: AbstractRelative, S, T}
   #
-  return _updateCCW!(F, ccwl, Xi, solvefor, N; kw...)
+  return _beforeSolveCCW!(F, ccwl, Xi, N; kw...)
 end
 
-function _updateCCW!(
+function _beforeSolveCCW!(
   ccwl::Union{CommonConvWrapper{F}, CommonConvWrapper{Mixture{N_, F, S, T}}},
   Xi::AbstractVector{<:DFGVariable},
-  solvefor::Symbol,
+  # solvefor::Symbol,
   N::Integer;
   kw...,
 ) where {N_, F <: AbstractPrior, S, T}
   #
-  return _updateCCW!(F, ccwl, Xi, solvefor, N; kw...)
+  return _beforeSolveCCW!(F, ccwl, Xi, N; kw...)
 end
 
 

src/services/DeconvUtils.jl

@@ -55,7 +55,7 @@ function approxDeconv(
   fctSmpls = deepcopy(measurement)
 
   # TODO assuming vector on only first container in measurement::Tuple
-  makeTarget = (i) -> measurement[i] # TODO does not support copy-primitive types like Float64, only Ref()
+  makeTarget = (smpidx) -> measurement[smpidx] # TODO does not support copy-primitive types like Float64, only Ref()
   # makeTarget = (i) -> view(measurement[1][i],:)
   # makeTarget = (i) -> view(measurement[1], :, i)
 
@@ -73,22 +73,22 @@ function approxDeconv(
 
   for idx = 1:N
     # towards each particle in their own thread (not 100% ready yet, factors should be separate memory)
-    targeti_ = makeTarget(idx)
+    target_smpl = makeTarget(idx)
 
     # TODO must first resolve hypothesis selection before unrolling them -- deferred #1096
     resize!(ccw.activehypo, length(hyporecipe.activehypo[2][2]))
     ccw.activehypo[:] = hyporecipe.activehypo[2][2]
 
-    onehypo!, _ = _buildCalcFactorLambdaSample(ccw, idx, targeti_, measurement)
+    onehypo!, _ = _buildCalcFactorLambdaSample(ccw, idx, target_smpl, measurement)
     #
 
     # lambda with which to find best measurement values
-    hypoObj = (tgt) -> (targeti_ .= tgt; onehypo!())
+    hypoObj = (tgt) -> (target_smpl .= tgt; onehypo!())
 
     # find solution via SubArray view pointing to original memory location
     if fcttype isa AbstractManifoldMinimize
       sfidx = ccw.varidx[]
-      targeti_ .= _solveLambdaNumericMeas(
+      target_smpl .= _solveLambdaNumericMeas(
         fcttype,
         hypoObj,
         res_,
@@ -97,7 +97,7 @@ function approxDeconv(
         islen1,
       )
     else
-      targeti_ .= _solveLambdaNumeric(fcttype, hypoObj, res_, measurement[idx], islen1)
+      target_smpl .= _solveLambdaNumeric(fcttype, hypoObj, res_, measurement[idx], islen1)
     end
   end