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

Refactor graph copy functions #300

Closed
Affie opened this issue Feb 14, 2020 · 3 comments · Fixed by #373
Closed

Refactor graph copy functions #300

Affie opened this issue Feb 14, 2020 · 3 comments · Fixed by #373
Labels
Milestone

Comments

@Affie
Copy link
Member

Affie commented Feb 14, 2020

Look at draft of copyGraph functions in attic/sandbox.jl
The idea is to have only one function to be called by functions such as buildSubgraphFromLabels

Will at least look at it for v0.6.0 and decide if it should be added.

@Affie Affie added the refactor label Feb 14, 2020
@Affie Affie modified the milestones: v0.6.0, v0.6.x, v0.6.1 Feb 14, 2020
@Affie
Copy link
Member Author

Affie commented Feb 21, 2020

related to #250
Will refactor and consolidate in the v0.6.x cycle

@dehann dehann modified the milestones: v0.6.1, v0.6.2 Feb 28, 2020
@dehann
Copy link
Member

dehann commented Feb 28, 2020

agree, buildSubgraphFromLabels should be using common DFG functions to minimize code maintenance. Might need a keyword or two for more use cases.

@Affie
Copy link
Member Author

Affie commented Mar 30, 2020

Current state

I don't quite get the there are currently 5 subgraph functions comment, but lets look at all related functions:

Currently we have _copyIntoGraph! as the core function.

  • getSubgraph (basically an alias for _copyIntoGraph!)
  • getSubgraphAroundNode (getNeighborhood -> _copyIntoGraph!)
    • can possibly be consolidated with getSubgraph
    • Maybe use build verb (buildSubgraph) and then use multiple dispatch for "AroundNode" part.

We then have 2 "IIF" functions, but I think its mostly because of a misunderstanding.

  • buildSubgraphFromLabels! (I don't think this function should exist at all, but I guess that's why we have 2)
  • buildCliqSubgraph! (This doesn't use _copyIntoGraph! yet, but its planned with its replacement deepcopyGraph!. see snipped bellow.

buildCliqSubgraph! builds a very unique subgraph with frontals and separators handled differently (so I believe it has a place as its own function in IIF). Part of the motivation is to replace the "copy" with a "view" in the future, ie. viewGraph.

function buildCliqSubgraph!(dfg::AbstractDFG,
                            cliqSubFg::AbstractDFG,
                            frontals::Vector{Symbol},
                            separators::Vector{Symbol};
                            solvable::Int=0)


  allvars = union(frontals,separators)

  #get list of factors to possibly add, ie. frontal neighbors
  addfac = Symbol[]
  for sym in frontals
    union!(addfac, getNeighbors(dfg,sym))
  end

  #filter to only the factors in the clique
  allfacs = Symbol[]
  for sym in addfac
    vos = getVariableOrder(dfg, sym)
    if vos  allvars   
      union!(allfacs, [sym])
    end
  end

  # add all the factors and variables to the clique subgraph
  deepcopyGraph!(cliqSubFg, dfg, union(allvars, allfacs))

  return cliqSubFg
end

Summary

I would suggest and will implement if there are no objections:

  • A core copy function. _copyIntoGraph! improved and renamed to copyGraph! with "aliases" such as deepcopyGraph! and Base.convert. Drivers can overwrite this function for efficiency. see 569c3a3
  • DFG function buildSubgraph generic subgraph helper methods for neighbourhoods etc. will call copyGraph! to do the work with labels created from neighbors and labels.
  • IIF specialized buildCliqSubgraph! function that builds a subgraph from frontals and separators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants