Refactor graph copy and subgraph functions

[Affie]

Close #300
Still to do:

• add tests for buildSubgraph
• deprecate _copyIntoGraph in favor of copyGraph/deepcopyGraph/deepcopyGraph!
• deprecate getSubgraph in favor of buildSubgraph
• deprecate getSubgraphAroundNode in favor of buildSubgraph

[dehann]

Punchline is there are two approaches to do a copy:

• mini-batch of all variables followed by all factors; or
• sequentially around each node in the graph (edge by edge).

Hi @GearsAD , is there a particular preference on approach for DB version? Johan is busy consolidating the various graph copy functions into one or two primary copy functions in DFG.

[dehann]

should this not perhaps be an inner constructor, or does it not matter ( I'm not sure about the best practices on inner or outer constructors at the moment)?

[Affie]

https://docs.julialang.org/en/v1/manual/constructors/#man-inner-constructor-methods-1

It is good practice to provide as few inner constructor methods as possible: only those taking all arguments explicitly and enforcing essential error checking and transformation. Additional convenience constructor methods, supplying default values or auxiliary transformations, should be provided as outer constructors that call the inner constructors to do the heavy lifting. This separation is typically quite natural.

[dehann]

got it thanks!

[dehann]

I think we can merge this, right? This is very cool, thanks @Affie !

[GearsAD]

LGTM, just one thing - is there any reason (other than the original function signatures) that we combine variables and factors as input to copyGraph!, e.g. variableFactorLabels::Vector{Symbol};?

This just results in the lists being split up first thing into the function. If there isn't a reason beyond the initial design, I think we should split it up to make the calls more efficient.

[GearsAD]

Btw, I meant that we can do that in a subsequent PR - I'll add that. This is good thanks.

[GearsAD]

Oh I see you add it to the list already :) If you haven't started, let's save it for another PR

[Affie]

Ok, will do separate PR. Just don't start using it in master yet.