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

How to do DFGFactor with incomplete subset DFGVariables #510

Closed
dehann opened this issue Jun 17, 2020 · 4 comments
Closed

How to do DFGFactor with incomplete subset DFGVariables #510

dehann opened this issue Jun 17, 2020 · 4 comments

Comments

@dehann
Copy link
Member

dehann commented Jun 17, 2020

Add an option to allow incomplete factors to be added. Example, on :x0 a prior and odo to :x1: Then buildSubgraph(fg, [:x0;], allFactors=true) should return [:x0f1]--(:x0)--[:x0x1f1] even though [:x0x1f1] is missing the other variable (:x1).

Default must be allFactors=false.

┌ Warning: Factor x0x1f1 will be an orphan in the destination graph, and therefore not added.
└ @ DistributedFactorGraphs ~/.julia/packages/DistributedFactorGraphs/RktZF/src/services/AbstractDFG.jl:904
@dehann dehann added this to the v0.9.0 milestone Jun 17, 2020
@Affie
Copy link
Member

Affie commented Jun 17, 2020

I'm curious, why the change of heart? What is the use case for it?

Also, remember orphaned factors are not supported in an FG at all. If we implement this here is the list of related issues to help:
#338
#371
#339
#325
#296
#250
#20
The biggest thing would be to allow a factor graph to support orphans again, but it's doable.

@dehann dehann modified the milestones: v0.9.0, v0.x.0, vX.0.0 Jun 17, 2020
@dehann dehann changed the title buildSubgraph option allFactors=true How to do DFGFactor with incomplete subset DFGVariables Jun 17, 2020
@dehann
Copy link
Member Author

dehann commented Jun 17, 2020

why the change of heart

It's about capability and what is needed as things develop. We don't need orphaned factors to work, but I think there are fringe cases where it might useful to be able to have an incomplete graph. E.g. visualization is an obvious one -- draw this variable and associated factors only. I do recall we spoke about this at length on the phone. There is no rush to do this now, but lets leave the issue open and see if others are interested.

orphaned factors are not supported in an FG at all

Is this because we made variableOrder immutable?

allow a factor graph to support orphans again

Let's at least document why?

  • Performance has driven towards immutable DFGFactor and DFGVariable and we are avoiding heap lookups during several vital functions?
    • We don't want to allow factors to be added first and then be modified every time a variable is added, right?
  • Or more severe, like a deeply routed assumption on variableOrder must always be a hard-perfect reflection of what is in the factor graph?

I recall the main issue is around copySubgraph! which (again for performance) does variables first and then adds factors only when all variableOrder variables are already there -- i.e. warnings in (#510 (comment)).

I'd definitely want to keep DFGFactor immutable.

Potential workarounds (later if we decide to add something or a workaround), and :

  • allow subgraph to be incomplete wrt all variableOrders from its factors (as a special case, or with special flag)? Derived functions should check for the flag or continue as normal.
    • Problem is that would likely be a heap lookup every time, yuck...
  • Or, neighbor count checks, yuck...
  • E.g. visualization is an obvious one -- draw this variable and associated factors only
    • Perhaps we can solve that by suppressing "unwanted" variable nodes in the few cases like visualization (outside of the main DFG plumbing). Can add a skip list to the draw function, and then just pass that and get the same picture?
    • DF 👍

I'd vote in v1.x we do the third option, use tricks like skipList in the final user function with this hard documented reason on why we don't support incomplete DFGFactors.

@Affie
Copy link
Member

Affie commented Jun 17, 2020

The origin of removing orphans was that big variable order bug #337.
So it just needs a bunch of functions to be supported properly. We didn’t see the need back then.

If it’s only for visualization I’d say a view out filter approach would work fine. If there are other use cases we can go ahead and implement orphaned factors.

variableorder should remain immutable.

@dehann dehann modified the milestones: v0.x.0, v0.8.1 Jun 17, 2020
@dehann
Copy link
Member Author

dehann commented Jun 17, 2020

Thanks, think that is it then. Lets do "user must filter out" approach until there is enough demand to change. We can reopen the issue if folks continue to pile on requests in this thread.

We didn’t see the need back then.

That's perfect, lets see how far we get with current structure.

So it just needs a bunch of functions to be supported properly.

variableorder should remain immutable.

Sounds like the workaround would be more refinement of functions than big changes in types, so we should be good for v1.x. Thanks!

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

No branches or pull requests

2 participants