-
Notifications
You must be signed in to change notification settings - Fork 34
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
Return a mixture of exact and sampled weights as appropriate #255
Conversation
Pull Request Test Coverage Report for Build 5468581107
💛 - Coveralls |
[ci skip]
`math.nextafter` wasn't introduced until Python 3.9
[ci skip]
[ci skip]
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.
Looking great, made a few comments and will come back to this for another pass in a bit.
circuit_knitting/cutting/qpd/qpd.py
Outdated
Generate weights from the joint quasiprobability distribution. | ||
All weights above ``1 / num_samples`` will be returned exactly, while the | ||
remaining weights will be sampled from. |
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.
remaining weights will be sampled from. | |
remaining weights will be generated by sampling from the low-probability elements of the distribution. |
I think something like this is a little more clear. What do you think?
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 rephrased this in 8830c94.
circuit_knitting/cutting/qpd/qpd.py
Outdated
qpd_bases: The :class:`QPDBasis` objects from which to sample | ||
num_samples: Number of random samples to generate | ||
qpd_bases: The :class:`QPDBasis` objects from which to generate weights | ||
num_samples: Maximum number of weights to generate |
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.
num_samples: Maximum number of weights to generate | |
num_samples: Number of times to sample from the quasiprobability distribution |
I see what you're getting at here, and it makes sense for the CX/CZ case, but I think the description of this arg is more informative if you make it a bit more general
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 also rephrased this a bit in 8830c94.
[ci skip]
[ci skip]
[ci skip]
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.
Looks great. A couple of pedantic suggestions, but LGTM 👍
Fixes #199
Remaining action items:
generate_qpd_weights