Skip to content

Commit ddf99a4

Browse files
AffieCopilot
andauthored
Standardize delete! and merge! (#1212)
* delete return 0 for missing entries and merge functions * Pin StructUtils, deprecate buildSubgraph, and better merge/delete * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix typos and GraphsDFGs import/using * Fix/update docs * fix deleteBlobentries! return --------- Co-authored-by: Johannes Terblanche <Affie@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent f1cea48 commit ddf99a4

File tree

21 files changed

+740
-283
lines changed

21 files changed

+740
-283
lines changed

Project.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ SHA = "0.7, 1"
6565
SparseArrays = "1.11"
6666
StaticArrays = "1"
6767
Statistics = "1.11"
68-
StructUtils = "2.6.0"
68+
StructUtils = "=2.7.0"
6969
Tables = "1.11.1"
7070
Tar = "1.9"
7171
TensorCast = "0.3.3, 0.4"

docs/src/BuildingGraphs.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ To list all variables or factors (instead of just their labels), use the
116116
Traversing and Querying functions for finding the relationships and building subtraphs include:
117117

118118
- [`listNeighbors`](@ref)
119-
- [`buildSubgraph`](@ref)
119+
- [`getSubgraph`](@ref)
120120
- [`getBiadjacencyMatrix`](@ref)
121121

122122
## Getting (Reading) Variables and Factors

src/DataBlobs/services/BlobEntry.jl

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ function mergeBlobentry!(node, entry::Blobentry)
4646
end
4747

4848
function mergeBlobentries!(node, entries::Vector{Blobentry})
49+
#TODO optimize with something like: merge!(refBlobentries(node), entries)
4950
mergeBlobentry!.(node, entries)
5051
return length(entries)
5152
end
@@ -54,16 +55,15 @@ end
5455
$(SIGNATURES)
5556
"""
5657
function deleteBlobentry!(node, label::Symbol)
57-
!haskey(refBlobentries(node), label) && throw(LabelNotFoundError("Blobentry", label))
58+
!haskey(refBlobentries(node), label) && return 0
5859
pop!(refBlobentries(node), label)
5960
return 1
6061
end
6162

6263
deleteBlobentry!(node, entry) = deleteBlobentry!(node, getLabel(entry))
6364

6465
function deleteBlobentries!(node, labels::Vector{Symbol})
65-
deleteBlobentry!.(node, labels)
66-
return length(labels)
66+
return sum(deleteBlobentry!.(node, labels))
6767
end
6868

6969
"""

src/DataBlobs/services/BlobStores.jl

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -172,15 +172,12 @@ function deleteBlob!(store::FolderStore{T}, blobid::UUID) where {T}
172172
rm(blobfilename)
173173
# Create a tombstone marker
174174
open(tombstonefile, "w") do f
175-
return write(f, "deleted")
175+
return write(f, string("DELETED: ", now(UTC)))
176176
end
177177
return 1
178-
elseif isfile(tombstonefile)
179-
# Already deleted
180-
return 0
181178
else
182-
# Not found
183-
throw(IdNotFoundError("Blob", blobid))
179+
# Already deleted or doesn't exist
180+
return 0
184181
end
185182
end
186183

@@ -235,9 +232,7 @@ function addBlob!(store::InMemoryBlobstore{T}, blobid::UUID, data::T) where {T}
235232
end
236233

237234
function deleteBlob!(store::InMemoryBlobstore, blobid::UUID)
238-
if !haskey(store.blobs, blobid)
239-
throw(IdNotFoundError("Blob", blobid))
240-
end
235+
!haskey(store.blobs, blobid) && return 0
241236
pop!(store.blobs, blobid)
242237
return 1
243238
end
@@ -372,9 +367,7 @@ function addBlob!(store::RowBlobstore{T}, blobid::UUID, blob::T) where {T}
372367
end
373368

374369
function deleteBlob!(store::RowBlobstore, blobid::UUID)
375-
if !haskey(store.blobs, blobid)
376-
throw(IdNotFoundError("Blob", blobid))
377-
end
370+
!haskey(store.blobs, blobid) && return 0
378371
pop!(store.blobs, blobid)
379372
return 1
380373
end

src/Deprecated.jl

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
export FactorCompute
55
const FactorCompute = FactorDFG
66

7+
#TODO maybe keep for Bloblet type assert later.
78
const MetadataTypes = Union{
89
Int,
910
Float64,
@@ -424,3 +425,205 @@ function getCoordinates(
424425
X = ManifoldsBase.log(M, p0, p)
425426
return ManifoldsBase.get_coordinates(M, p0, X, basis)
426427
end
428+
429+
# old merge and copy functions, will be replaced by cleaner merge!, sync!, and copyto! functions
430+
431+
# """
432+
# $(SIGNATURES)
433+
# Merger sourceDFG to destDFG given an optional list of variables and factors and distance.
434+
# Notes:
435+
# - Nodes already in the destination graph are updated from sourceDFG.
436+
# - Orphaned factors (where the subgraph does not contain all the related variables) are not included.
437+
# Related:
438+
# - [`copyGraph!`](@ref)
439+
# - [`buildSubgraph`](@ref)
440+
# - [`listNeighborhood`](@ref)
441+
# - [`deepcopyGraph`](@ref)
442+
# """
443+
444+
##==============================================================================
445+
## Copy Functions #TODO replace with sync
446+
##==============================================================================
447+
448+
"""
449+
$(SIGNATURES)
450+
Common function for copying nodes from one graph into another graph.
451+
This is overridden in specialized implementations for performance.
452+
Orphaned factors are not added, with a warning if verbose.
453+
Set `overwriteDest` to overwrite existing variables and factors in the destination DFG.
454+
NOTE: `copyGraphMetadata` is deprecated – use agent/graph Bloblets instead.
455+
Related:
456+
- [`deepcopyGraph`](@ref)
457+
- [`deepcopyGraph!`](@ref)
458+
- [`buildSubgraph`](@ref)
459+
- [`listNeighborhood`](@ref)
460+
- [`mergeGraph!`](@ref)
461+
"""
462+
#
463+
function copyGraph!(
464+
destDFG::AbstractDFG,
465+
sourceDFG::AbstractDFG,
466+
variableLabels::AbstractVector{Symbol} = listVariables(sourceDFG),
467+
factorLabels::AbstractVector{Symbol} = listFactors(sourceDFG);
468+
copyGraphMetadata::Bool = false,
469+
overwriteDest::Bool = false,
470+
deepcopyNodes::Bool = false,
471+
verbose::Bool = false,
472+
showprogress::Bool = verbose,
473+
)
474+
# Split into variables and factors
475+
sourceVariables = getVariables(sourceDFG, variableLabels)
476+
sourceFactors = getFactors(sourceDFG, factorLabels)
477+
# Now we have to add all variables first,
478+
@showprogress desc = "copy variables" enabled = showprogress for variable in
479+
sourceVariables
480+
481+
variableCopy = deepcopyNodes ? deepcopy(variable) : variable
482+
if !hasVariable(destDFG, variable.label)
483+
addVariable!(destDFG, variableCopy)
484+
elseif overwriteDest
485+
mergeVariable!(destDFG, variableCopy)
486+
else
487+
throw(LabelExistsError("Variable", variable.label))
488+
end
489+
end
490+
# And then all factors to the destDFG.
491+
@showprogress desc = "copy factors" enabled = showprogress for factor in sourceFactors
492+
# Get the original factor variables (we need them to create it)
493+
sourceFactorVariableIds = collect(factor.variableorder)
494+
# Find the labels and associated variables in our new subgraph
495+
factVariableIds = Symbol[]
496+
for variable in sourceFactorVariableIds
497+
if hasVariable(destDFG, variable)
498+
push!(factVariableIds, variable)
499+
end
500+
end
501+
# Only if we have all of them should we add it (otherwise strange things may happen on evaluation)
502+
if length(factVariableIds) == length(sourceFactorVariableIds)
503+
factorCopy = deepcopyNodes ? deepcopy(factor) : factor
504+
if !hasFactor(destDFG, factor.label)
505+
addFactor!(destDFG, factorCopy)
506+
elseif overwriteDest
507+
mergeFactor!(destDFG, factorCopy)
508+
else
509+
throw(LabelExistsError("Factor", factor.label))
510+
end
511+
elseif verbose
512+
@warn "Factor $(factor.label) will be an orphan in the destination graph, and therefore not added."
513+
end
514+
end
515+
516+
if copyGraphMetadata
517+
error(
518+
"copyGraphMetadata keyword has been removed – metadata APIs were replaced by Bloblets. " *
519+
"Copy agent/graph Bloblets manually before calling copyGraph!",
520+
)
521+
end
522+
return nothing
523+
end
524+
525+
"""
526+
$(SIGNATURES)
527+
Copy nodes from one graph into another graph by making deepcopies.
528+
see [`copyGraph!`](@ref) for more detail.
529+
Related:
530+
- [`deepcopyGraph`](@ref)
531+
- [`buildSubgraph`](@ref)
532+
- [`listNeighborhood`](@ref)
533+
- [`mergeGraph!`](@ref)
534+
"""
535+
#
536+
function deepcopyGraph!(
537+
destDFG::AbstractDFG,
538+
sourceDFG::AbstractDFG,
539+
variableLabels::Vector{Symbol} = ls(sourceDFG),
540+
factorLabels::Vector{Symbol} = lsf(sourceDFG);
541+
kwargs...,
542+
)
543+
return copyGraph!(
544+
destDFG,
545+
sourceDFG,
546+
variableLabels,
547+
factorLabels;
548+
deepcopyNodes = true,
549+
kwargs...,
550+
)
551+
end
552+
553+
"""
554+
$(SIGNATURES)
555+
Copy nodes from one graph into a new graph by making deepcopies.
556+
see [`copyGraph!`](@ref) for more detail.
557+
Related:
558+
- [`deepcopyGraph!`](@ref)
559+
- [`buildSubgraph`](@ref)
560+
- [`listNeighborhood`](@ref)
561+
- [`mergeGraph!`](@ref)
562+
"""
563+
#
564+
function deepcopyGraph(
565+
::Type{T},
566+
sourceDFG::AbstractDFG,
567+
variableLabels::Vector{Symbol} = ls(sourceDFG),
568+
factorLabels::Vector{Symbol} = lsf(sourceDFG);
569+
graphLabel::Symbol = Symbol(getGraphLabel(sourceDFG), "_cp_$(string(uuid4())[1:6])"),
570+
kwargs...,
571+
) where {T <: AbstractDFG}
572+
destDFG = T(;
573+
solverParams = getSolverParams(sourceDFG),
574+
graph = sourceDFG.graph,
575+
agent = sourceDFG.agent,
576+
graphLabel,
577+
)
578+
copyGraph!(
579+
destDFG,
580+
sourceDFG,
581+
variableLabels,
582+
factorLabels;
583+
deepcopyNodes = true,
584+
kwargs...,
585+
)
586+
return destDFG
587+
end
588+
589+
function mergeGraph!(
590+
destDFG::AbstractDFG,
591+
sourceDFG::AbstractDFG,
592+
variableLabels::Vector{Symbol},
593+
factorLabels::Vector{Symbol} = lsf(sourceDFG),
594+
distance::Int = 0;
595+
solvableFilter = nothing,
596+
tagsFilter = nothing,
597+
kwargs...,
598+
)
599+
Base.depwarn(
600+
"""
601+
mergeGraph! with variableLabels, factorLabels, and distance is deprecated.
602+
For now use function composition ending with mergeGraph!,
603+
syncGraph!(Coming soon) will replace this functionality.
604+
""",
605+
:mergeGraph!,
606+
)
607+
# find neighbors at distance to add
608+
sourceVariables, sourceFactors = listNeighborhood(
609+
sourceDFG,
610+
union(variableLabels, factorLabels),
611+
distance;
612+
solvableFilter,
613+
tagsFilter,
614+
)
615+
616+
copyGraph!(
617+
destDFG,
618+
sourceDFG,
619+
sourceVariables,
620+
sourceFactors;
621+
deepcopyNodes = true,
622+
overwriteDest = true,
623+
kwargs...,
624+
)
625+
626+
return destDFG
627+
end
628+
629+
@deprecate buildSubgraph(args...; kwargs...) getSubgraph(args...; kwargs...)

src/DistributedFactorGraphs.jl

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export AbstractStateType, StateType
7777
##------------------------------------------------------------------------------
7878
## Types
7979
##------------------------------------------------------------------------------
80-
#TODO types are not yet stable - also, we might not export types such as VariableCompute
80+
#TODO types are not yet stable - also, we might not export all types
8181
# Variables
8282
export VariableDFG
8383
export VariableSummary
@@ -181,7 +181,7 @@ export listFactors
181181
export listStates
182182

183183
##
184-
export getObservation
184+
public getObservation
185185

186186
##------------------------------------------------------------------------------
187187
# Tags
@@ -307,10 +307,10 @@ export listFactorBloblets
307307
export listGraphBloblets
308308
export listAgentBloblets
309309

310-
# export hasVariableBloblet
311-
# export hasFactorBloblet
312-
# export hasGraphBloblet
313-
# export hasAgentBloblet
310+
export hasVariableBloblet
311+
export hasFactorBloblet
312+
export hasGraphBloblet
313+
export hasAgentBloblet
314314

315315
## v1 name, signiture, and return
316316

@@ -485,7 +485,7 @@ const unstable_functions::Vector{Symbol} = [
485485
:natural_lt, #TODO do we export stable functions such as natural_lt or just mark as public
486486
:sortDFG, #TODO do we export stable functions such as natural_lt or just mark as public
487487
:mergeGraph!,
488-
:buildSubgraph,
488+
:getSubgraph,
489489
:incrDataLabelSuffix,# TODO somewhat used, do we deprecate?
490490

491491
# set # TODO what to do here, maybe `ref` verb + setproperty.
@@ -509,7 +509,7 @@ const unstable_functions::Vector{Symbol} = [
509509
:unpackDistribution,
510510
:hasTagsNeighbors,
511511
# :updateBlobstore!,## TODO deprecated or obsolete
512-
:emptyMetadata!, #TODO maybe deprecate for just deleteMetadata!
512+
# :emptyMetadata!, #TODO maybe deprecate for just deleteMetadata!
513513
# :emptyBlobstore!, #TODO maybe deprecate for just deleteBlobstore!
514514
:MetadataTypes, #maybe make public after metadata stable
515515
:getVariableTypeName,

src/GraphsDFG/GraphsDFG.jl

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ using ...DistributedFactorGraphs:
1111
Agent,
1212
LabelNotFoundError,
1313
LabelExistsError,
14+
MergeConflictError,
1415
Graphroot,
1516
AbstractGraphVariable,
1617
AbstractGraphFactor,
@@ -25,7 +26,8 @@ using ...DistributedFactorGraphs:
2526
Blobentries,
2627
FolderStore,
2728
refTags,
28-
listTags
29+
listTags,
30+
patch!
2931

3032
# import DFG functions to extend
3133
import ...DistributedFactorGraphs:
@@ -53,7 +55,6 @@ import ...DistributedFactorGraphs:
5355
isConnected,
5456
listNeighbors,
5557
buildSubgraph,
56-
copyGraph!,
5758
getBiadjacencyMatrix,
5859
toDot,
5960
toDotFile,

0 commit comments

Comments
 (0)