-
Notifications
You must be signed in to change notification settings - Fork 81
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
Feature/cut variables #548
Conversation
from dimod.core.composite import ComposedSampler | ||
from dimod.sampleset import SampleSet | ||
import dimod | ||
import networkx as nx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See discussion in #539 (comment). However, in this case the functions are potentially more involved, so rewriting might be more trouble than it's worth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nx.articulation_points and nx.biconnected_components aren't too difficult actually, and pure python. It's basically just DFS. So if you'd rather have dimod.articulation_points and dimod.biconnected_components we could do that (although it does seem like a lot of code replication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly, e5dd5f3 added bfs so dfs as well is not such a stretch, but at some point we end up just re-implementing NetworkX and there's no point in that. On the other hand, casting BQMs to NetworkX graphs has a non-trivial copy cost so working natively on BQMs can be nice 🤷♂️.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we should try to use nx's function until it's clear there is a drastic slowdown because of copy costs or pure Python.
Glad to learn about nx.articulation_points
. I somehow missed the articulation_points
function last summer when I was working on this problem. That would have simplified things. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree. It's just frustrating because the BQM and networkx graph are so close in API, for a lot of nx functions the only thing blocking them working on the BQM directly is a decorator checking whether it's a directed graph or not. We could potentially wrap the BQM with some attributes to get around that but I am sure there would be edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I've had great results using Metis in the past to decompose graphs. IIRC, it has a Python interface, but would add another requirement (including C backend 👎 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pymetis, I think. Yeah metis is great (although overkill if you just want cut vertices rather a partition of an arbitrary graph). In principle we could extend the functionality in CutVertexComposite to cut sets larger than a single vertex, but the run time grows exponentially with the size of the cut set.
return tree_decomp.sample(self.child, **parameters) | ||
|
||
|
||
def sub_bqm(bqm, variables): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a comment on this PR per se, but linking to #538 for future reference
__all__ = ['CutVertexComposite'] | ||
|
||
|
||
class CutVertexComposite(ComposedSampler): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alex made a good point that the original name CutVariableComposite is more consistent with dimod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make things easy to search for, would it be too weird to have an alias like CutVertexComposite = CutVariableComposite
? Just because "cut vertex" is a more likely search term.
|
||
return T, root | ||
|
||
def sample(self, sampler, **parameters): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use self.child
as a sampler? This requires that BiconnectedTreeDecomposition
inherent from ComposedSampler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, you're already passing self.child
of CutVertexComposite
to this. Nevermind
Closing in favour of dwavesystems/dwave-preprocessing#36 |
Adds CutVertexComposite, which decomposes a connected bqm into biconnected components, solves those components, and merges the solution.
Currently the composite returns a single solution, like the ConnectedComponentsComposite. Eventually it should have the option of sampling.
Closes #547