Transfer clifford and cnot-dihedral synthesis functions from quantum_info to synthesis #9141
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 the following people are requested to review this:
|
ShellyGarion
changed the title
|
This PR is almost done (only needs release notes). I would like to approve the API before that. |
There was a problem hiding this comment.
Thanks @ShellyGarion. I agree that moving synthesis algorithms for Clifford and CNOTDihedral into the qiskit/synthesis directory makes a lot of sense and makes the code structure a bit cleaner. As far as I can judge, this PR does not do any algorithmic changes (modulo some minor renaming), so overall this looks good to me.
One minor question that I had is whether we still need clifford_decompose.py and cnotdihedral_decompose.py files in the quantum_info/synthesis directory. This is not by any means a suggestion to remove these files, it's only a question if you think we need them in the long run?
The renamed function names (such as synch_clifford_ag instead of decompose_clifford_ag) are completely fine, I don't think we really needed to rename these, but I do like the new version slightly better, and maybe we are implicitly converging to a convention that synthesis functions in the qiskit/synthesis directory should start with synth.
Since now Clifford and CNOTDihedral synthesis methods (such as synth_clifford_ag, synth_cnotdihedral_full, etc.) are part of the official documentation, it makes sense to add some release notes.
There was a problem hiding this comment.
Thank you. The direction is nice.
I'm concerned about circular dependencies between quantum_info and synthesis. I think it would be nice that synthesis depends on quantum_info and synthesis should not depend on quantum_info. It is good to be dependent only during the deprecation period. It is transient.
I only wrote comments in one place because I didn't want to write the same again, but there are many similar comments.
And also could you add tests for synthesis and release note?
| # --------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def decompose_clifford_bm(clifford): |
There was a problem hiding this comment.
Removing these public functions are API breaking, so could you add deprecation warning?
There was a problem hiding this comment.
These functions are internal and not part of the API. The only function appearing in the docs is decompose_clifford:
https://qiskit.org/documentation/apidoc/quantum_info.html#synthesis
There was a problem hiding this comment.
I found it in https://qiskit.org/documentation/apidoc/transpiler_synthesis_plugins.html?highlight=decompose_clifford_bm#high-level-synthesis-plugins.
The issue is what is a public API in Qiskit. More to the point, I think the question is whether there are users... (if you don't think there are no users, I'm fine)
There was a problem hiding this comment.
Thanks for catching this! I fixed the documentation accordingly.
|
Thank you @alexanderivrii and @ikkoham for your reviews! |
| # --------------------------------------------------------------------- | ||
|
|
||
|
|
||
| def decompose_clifford_bm(clifford): |
There was a problem hiding this comment.
I found it in https://qiskit.org/documentation/apidoc/transpiler_synthesis_plugins.html?highlight=decompose_clifford_bm#high-level-synthesis-plugins.
The issue is what is a public API in Qiskit. More to the point, I think the question is whether there are users... (if you don't think there are no users, I'm fine)
@ikkoham, thanks for spotting this. This is a really new feature, and I don't think anyone is really using it yet, so it should probably be ok to just change the documentation without issuing a deprecation warning. |
|
@alexanderivrii Thanks. I understand the situation. I think it's good. |
ikkoham
added
Changelog: Deprecation
…info to synthesis (Qiskit#9141) * transfer clifford synthesis functions from quantum_info to synthesis * add init file * transfer cnotdihedral synthesis from quantum_info to synthesis * rename clifford synth functions. add docs * add clifford synthesis methods to docs * add documentation for cnotdihedral synth methods * add decompose_cnotdihedral to quantum_info docs * deprecate decompose_clifford and decompose_cnotdihedral - move to qsikit.synthesis * style * add clifford and cnotdihedral decompose full to synthesis * enhance tests * update high_level_synthesis transpiler pass * add release notes * fix docs in synthesis plugin Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Transfer Clifford and CNOT-Dihedral synthesis functions from
qiskit/quantum_info/synthesistoqiskit/synthesisDetails and comments
The functions
decompose_cliffordanddecompose_cnotdihedralwill be deprecated, renamed and moved toqiskit/quantum_info/synthesis.