Include chain break points in returned embedding context#447
Include chain break points in returned embedding context#447kevinchern wants to merge 3 commits intodwavesystems:masterfrom
Conversation
arcondello
left a comment
arcondello
left a comment
There was a problem hiding this comment.
Thanks for this!
A few comments on the code.
Once that's sorted there might be a few more formatting comments on the docstring.
dwave/embedding/chain_breaks.py
Outdated
| """Identify breakpoints in each chain. | ||
|
|
||
| Args: | ||
| samples (array_like): |
There was a problem hiding this comment.
I think this should be expanded to samples like. You can use array, labels = dimod.as_samples(samples_like) to get the numpy array.
The reason I say this is because in Ocean you can get embeddings that look like {'a': ['b', 'c']}. The QPU (currently) only uses integer labels for its qubits, but that might change in the future.
Should add a test for this as well.
dwave/embedding/chain_breaks.py
Outdated
|
|
||
| Args: | ||
| samples (array_like): | ||
| Samples as a nS x nV array_like object where nS is the number of samples and nV is the |
There was a problem hiding this comment.
This should also specify that the samples should be for the embedded problem
dwave/embedding/chain_breaks.py
Outdated
| for sample in samples: | ||
| bps = {} | ||
| for node in embedding.keys(): | ||
| chain_edges = embedding.chain_edges(node) |
There was a problem hiding this comment.
to avoid some confusing errors, you probably want
try:
chain_edges = embedding.chain_edges(nodes)
except AttributeError:
raise TypeError("'embedding' must be a dwave.embedding.EmbeddedStructure") from None
| embedding_parameters=embedding_parameters, | ||
| chain_strength=embedding.chain_strength) | ||
| chain_strength=embedding.chain_strength, | ||
| break_points=break_points(response, embedding)) |
There was a problem hiding this comment.
This is a non-trivial performance hit. IMO we either should not do this by default or we need to write a more performant implementation of break_points.
There was a problem hiding this comment.
I agree, ideally this would be a lazy proxy.
But FWIW, return_embedding does default to False.
There was a problem hiding this comment.
That's true, unless for instance the inspector is imported.
My inclination is to not include this in the embedding composite for now, but document how to use the TrackingComposite to get the information in the docstring of break_points().
There was a problem hiding this comment.
I updated the PR according to feedback except for this comment. Should I move the statistic to TrackingComposite instead?
I think the lazy proxy approach would require storing response. For a more performant implementation, I can wrap it in numba. Any suggestions for writing a more performant implementation?
There was a problem hiding this comment.
I think the best thing is just to remove the statistic from the embedding composite altogether. I would then add an example to the break_points docstring showing how to calculate it, using the TrackingComposite to retrieve the relevant information.
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
==========================================
- Coverage 90.52% 87.57% -2.96%
==========================================
Files 22 22
Lines 1520 1521 +1
==========================================
- Hits 1376 1332 -44
- Misses 144 189 +45
Continue to review full report at Codecov.
|
4 participants
Example of "chain break points":
a chain with +++: no break points.
a chain with +--: break point at (0,1)
a chain with +-+: break points at (0,1) and (1,2)