-
Notifications
You must be signed in to change notification settings - Fork 65
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
add LinearAncillaComposite #530
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #530 +/- ##
==========================================
- Coverage 89.65% 86.57% -3.09%
==========================================
Files 24 25 +1
Lines 1760 1832 +72
==========================================
+ Hits 1578 1586 +8
- Misses 182 246 +64 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
This can be very useful, @pau557. I made a few comments inline.
Needs to be added to docs here: https://github.com/dwavesystems/dwave-system/blob/master/docs/reference/composites.rst
releasenotes/notes/add-linear-ancilla-composite-3281ed6733b0f0c7.yaml
Outdated
Show resolved
Hide resolved
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.
Thank you
Any more comments or ready to merge? |
I had missed many comments, apologies @JoelPasvolsky. I think everything is addressed now |
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.
Just a few small comments.
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.
LGTM
bqm: dimod.BinaryQuadraticModel, | ||
*, | ||
h_tolerance: numbers.Number = 0, | ||
default_flux_bias_range: Sequence[float] = (-0.005, 0.005), |
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.
why not be more precise with:
default_flux_bias_range: Sequence[float] = (-0.005, 0.005), | |
default_flux_bias_range: tuple[float, float] = (-0.005, 0.005), |
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.
CircleCI didn't like that on python 3.8...
File "/home/circleci/project/dwave/system/composites/linear_ancilla.py", line 96, in LinearAncillaComposite
default_flux_bias_range: tuple[float, float] = (-0.005, 0.005),
TypeError: 'type' object is not subscriptable
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.
Yup. But we dropped support for 3.8 in the main branch since you opened this PR.. So, best to rebase to master
.
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.
@pau557, would you prefer to rebase, or should I do it?
@pau557, I think we can merge this when docs comments are addressed. |
How do I tell CircleCI not to run this example with a mock sampler?
|
You can mock the flux_biases parameter with h if you want this to behave correctly. |
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.
Couple of suggestions, these needn't be blockers to integration.
flux_biases = [0] * qpu_properties["num_qubits"] | ||
|
||
_bqm = bqm.copy() | ||
used_ancillas = defaultdict(list) |
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.
In practice we may want to share ancillas. This might allow some problems to be programmed that are impossible with unique ancilla per variable. Might be worth adding a comment to this effect somewhere, with a view to a future feature expansion (probably an optional argument).
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.
We share ancillas. We're just biased towards picking the least used ones
import networkx as nx | ||
import numpy as np | ||
|
||
from dwave.system.testing import MockDWaveSampler |
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 MockSampler can be amended so as to emulate flux_biases as Ising model linear biases:
https://github.com/AndyZzzZzzZzz/shimming-tutorial/blob/andy/tutorial_code/helpers/sampler_wrapper.py
I recommend you use a wrapper like this one and then test optimization using MockSampler().sample
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 tests in test_linear_ancilla_composite
do not check for sample correctness. Just for the implementation of ancillas and flux biases in the call. So the MockSampler works for it.
The error that I posted yesterday was related to circle CI testing the example in a docstring. In that case I am not sure it's possible to wrap with this fancier mock sampler. If it's not trivial to do so I would skip
Looks like an incorrect rebase: did you miss pulling the latest master branch? |
Yes, I did it wrong. @randomir is helping with it. |
Because this PR was opened from a fork's master branch, rebasing would be messy, i.e. history would be pretty much lost. In order to keep the conversation here relevant (and retain @pau557's authorship of commits), I have rebased in a new branch and verified all tests pass in a separate draft PR, #546. So we can merge this as-is. |
In scenarios where the h biases are not available or sufficient in range, one can use auxiliary qubits polarized with a large flux bias and coupled to the data qubit. This method:
This PR adds a LinearAncillaComposite that implements this technique.
For the reviewers, please take into account future extensions of this method:
Please educate me on how to use the correct Sphynx syntax