-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Fixes indexing of circuits in local folding methods for Cirq, adds tests #10
Conversation
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.
Nice work @rmlarose! Looks solid.
Had some small changes and then one suggestion about adding mutability for performance that would be good to get your thoughts on.
mitiq/folding_cirq.py
Outdated
|
||
for _ in range(num_to_fold): | ||
# TODO: This allows for gates to be folded more than once, and folded gates to be folded. |
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.
My intuition is that gates should be allowed to be sampled more than once, but that one should not sample from the new folds. Do you agree @andreamari I think this change is made doing:
moment_index = np.random.choice(len(circuit))
gate_index = np.random.choice(len(circuit[moment_index]))
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 have the same intuition, not 100% sure though. I'll wait to hear Andrea's and other's thoughts, it's not too hard to change either way.
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.
My intuition is that it is better to distribute the folding gates as much as possible along the circuit, so I think it is better not to sample the same gate of the original circuit (unless stretch >3
) AND not to sample previous folds.
What is the optimal way of doing this I don't know. I need to think about.
[I edited this comment. Please ignore my previous version since I was wrong about the creation of new moments.]
…ring to G G^dag G (before it was G^dag G G), re-writes tests.
…uality testing using CircuitDAGs.)
Addresses all points except folding gates that have already been folded, as I don't think we've reached a consensus on this. (Have we?) I've kept the "TODO" notes about this in the file as points to address once we reach a conclusion. I think the previous solution using **kwargs in the fold_method was more elegant, but this is a working version. Another possibility is to use *args. |
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.
Very minor final comments. Then likely is go to go
Additionally I opened #12 for discussion on resampling folded gates |
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.
For sure the methods: unitary_folding
, fold_local
, fold_gates_at_random
, fold_gates_from_left
,
should be public. What about if we make all the other ones, private methods? Just to create less confusion to the user. Moreover in this way they could all change circuits in place without problems, if necessary.
This is just a subjective idea, so feel free to ignore it.
I just noticed that the method |
…put circuit, refactors test cases. [WIP] Adds function for tracking moment indices to be used in fold_gates_at_random folding function.
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 good to me.
# This is the 1st commit message: simplify cirq getting started and add qiskit getting started # This is the commit message #2: add tests of qiskit getting started # This is the commit message #3: update qiskit example with conversions # This is the commit message #4: move seeds # This is the commit message #5: add doctests to main getting started # This is the commit message #6: better seed # This is the commit message #7: set seeds # This is the commit message #8: update # This is the commit message #9: qiskit name # This is the commit message #10: line # This is the commit message #11: update doc tests and remove redundant tests # This is the commit message #12: fix apidoc
* # This is a combination of 12 commits. # This is the 1st commit message: simplify cirq getting started and add qiskit getting started # This is the commit message #2: add tests of qiskit getting started # This is the commit message #3: update qiskit example with conversions # This is the commit message #4: move seeds # This is the commit message #5: add doctests to main getting started # This is the commit message #6: better seed # This is the commit message #7: set seeds # This is the commit message #8: update # This is the commit message #9: qiskit name # This is the commit message #10: line # This is the commit message #11: update doc tests and remove redundant tests # This is the commit message #12: fix apidoc * update getting started * reformat testcode as block * switch to make doctest * include change to make doctest in workflow file * add doctest to workflow * test documentation in ci with doctest * remove lines to prompt build * revert to original ci file * add new name run with pytest-sphinx * add sphinx to install * add cd * add sphinx dependencies and extentions to ci * purposefully introduce error to check ci build fails * revert to passing build * add documentation testing to readme * remove README extra file * update zne.py for new keep type default * move random seed to test * make test factories seed local * make noise mitigation more clear Co-authored-by: Nathan Shammah <nathan.shammah@gmail.com>
Fixes #8