-
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
Add a generate_cutting_experiments function #378
Conversation
Pull Request Test Coverage Report for Build 5932193014
💛 - Coveralls |
I'll just comment on this part, which is: once finalized and approved, we can merge this to |
--- | ||
features: | ||
- | | ||
Added a function, :func:`~circuit_knitting.cutting.generate_cutting_experiments`, which generates the cutting subexperiments to be run on the backend - given an input circuit, observable, and number of samples to draw from the input circuit. For each subexperiment, a quasi-distribution will be generated using Qiskit Sampler primitive. :func:`generate_cutting_experiments` also generates the weights associated with each subexperiment. The quasi-distributions and weights are used during expectation value reconstruction. |
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.
"samples to draw from the quasi-distribution representing the cuts." maybe? hmm
@garrison , this should be ready to be looked at. It's just moving a function and exposing it to the public API. It also adds unit tests, since this function isn't covered by the e2e tests. This function isn't used anywhere in the software or tutorials. It will be used in a later PR when we remove |
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.
Thanks!
Attached are my comments at the moment, though I am going to have to work with this a bit to tell which of the remaining code (if any) is new, or rather the few details that have been modified. In general, it's easier to review if the pull requests that change code/interfaces are distinct from the ones that purely move code. This is fine -- I can definitely work with this -- but ideally it would have been
- Remove the underscore to make the function public, add docstring(s), and add tests in the location appropriate for the code after the move (first PR). In the PR's rationale, mention which functions/classes/etc. you plan to move (and where) in a subsequent PR. This will minimize the diff in this stage to actual code changes.
- Actually move the code itself, without changing anything other than import lines (second PR)
This would also have avoided the brief period in which the function is duplicated (public & private versions in the repo at once).
will be removed along with the module
cutting_evaluation
in a following PR.
I wonder if we can/should keep execute_experiments
only as a deprecated function? (and remove it in a subsequent release)
We could do that, but it would no longer consume the output of |
Closing in favor of #385 |
This should be the first PR to be merged into
main
based on #352 .This creates a public function
generate_cutting_experiments
which largely mimics the current private function of the same name incutting_evaluation
. This function is being moved tocutting_decomposition
, as subexperiments will be created in that module starting in 0.4.0.This PR should not be merged prior to 0.3.1, so I am putting it on hold.
The private
_generate_cutting_experiments
will be removed along with the modulecutting_evaluation
in a following PR.