SynthesizeRZRotations pass #15641
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
|
|
Pull Request Test Coverage Report for Build 22852088886Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Janani, this is an amazing start. After several preliminary experiments, the code seems to work and return correct results.
I have left a few mostly minor comments. The suggestions are for illustration purposes only, so do not commit them directly through the web interface 😄.
| """Replace RZ gates with Clifford+T decompositions using gridsynth. | ||
|
|
||
| This pass replaces all single-qubit RZ rotation gates with sequences | ||
| of Clifford+T gates (H, S, T gates) using the gridsynth algorithm. |
There was a problem hiding this comment.
Also here: the (H, S, T) are specific to gridsynth.
|
|
||
| # [TO DO] could combine tests for angle and epsilon | ||
|
|
||
| @data(1e-9, 1e-10, 1e-11) |
There was a problem hiding this comment.
Maybe try lower values, like 1e-6? (If I remember correctly, the argument passed to the inner rust function is currently ignored).
There was a problem hiding this comment.
Noted Sasha. As you mentioned, I also just noticed that it ignores the argument and used the constant EPSILON, which may also be why this test passes now. I have changed how it accepts epsilon, will need to check if this test will still pass for variable epsilon.
There was a problem hiding this comment.
What is the difference between this and the following test?
There was a problem hiding this comment.
here you check that the operators are equal. in the following test it's up to an error.
|
An additional random idea while looking at the code (but let's implement it only in a follow-up, if at all). When considering the set of angles that need to be synthesized, you are already exploiting the fact that |
…ng synthesis of same angles
ShellyGarion
left a comment
There was a problem hiding this comment.
This PR looks almost ready now. I only have some minor comments and questions.
Thanks @jan-an and @alexanderivrii !
|
|
||
| The synthesis accuracy can be controlled by either specifying an | ||
| ``approximation_degree``, or alternatively by explicitly setting | ||
| both ``synthesis_error`` and ``cache_error``. |
There was a problem hiding this comment.
I would suggest to give in the API docstring an example for using synthesis_error and cache_error
There was a problem hiding this comment.
I could do that, but I think we are no longer adding large examples to the release notes. I also feel that most users (except for Julien) will not be interested in controlling the two parts of the error budget separately, so I would prefer not to stress out this use-case.
|
|
||
| # [TO DO] could combine tests for angle and epsilon | ||
|
|
||
| @data(1e-9, 1e-10, 1e-11) |
There was a problem hiding this comment.
here you check that the operators are equal. in the following test it's up to an error.
| @ddt | ||
| class TestSynthesizeRzRotations(QiskitTestCase): | ||
| """Test Synthesize Rz rotations method""" | ||
|
|
There was a problem hiding this comment.
perhaps it's worth to add tests for the parameters synthesis_error and cache_error ?
|
Hi @alexanderivrii , thanks for the clippy fix, indeed it was complaining that there needed to be an indent on the etc in docstring, which is fixed now. All changes look good to me, I just had one suggestion regarding the recent addition of the error budget; I think it would be good to have a test for calling the pass with the synthesis and cache errors. What do you think? |
I have removed the test that checks Operator equality for approximation degree below 1e-8 since I thought it's redundant (and depends a bit too much on the inner knowledge of error handling in Operator equality). In any case, we have plenty of tests that Operators are considered equal for the default value of approximation degree, and we have tests for the operator spectral norm when varying approximation degree. |
ShellyGarion
left a comment
There was a problem hiding this comment.
LGTM. thanks @jan-an and @alexanderivrii !
@Cryoris - do you have any further comments?
|
Yes, a few, I'm pushing a commit in a bit to address them. |
- our derivations use operator norm, not frobenius norm - use exact angle computation with arcsin - reduce some of the tests and add a real-life QFT one - use approximation_degree None instead of arithmetic in the signature - use PyResult over anyhow since that was the only result type
| let total_error = if let Some(approximation_degree) = approximation_degree { | ||
| MINIMUM_EPSILON.max(1. - approximation_degree) | ||
| } else { | ||
| 1e-10 |
There was a problem hiding this comment.
should it be hard-coded here or set some constant?
why here it's 1e-10 while MINIMUM_EPSILON is 1e-12?
There was a problem hiding this comment.
I moved it into a hardcoded constant. 1e-10 is a heuristic choice and tradeoff between accuracy and runtime. There's some inherent approximation we're using here and "no approximation" doesn't really make sense anymore here..
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Julien. A few minor comments on my part.
alexanderivrii
left a comment
There was a problem hiding this comment.
Great work, everyone: Janani - for implementing this pass, and Julien, Shelly & myself - for helping to push it across the finishing line.
RZSynthesispass with caching of Clifford+T synthesis results #15455This pass performs a two-fold optimization for implementing RZ synthesis:
to partition the