Improve LinearFunction synthesis #8568

ShellyGarion · 2022-08-17T09:48:07Z

Summary

Details and comments

move graysynth.py from qiskit.transpiler to qiskit.synthesis.linear
move test_synthesis.py from test/python/transpiler- where?
add a function that tries to synthesize A, A^{-1}, A^t and A^{-t} and choose the best CX cost and/or depth
add a util function for inverting a linear binary matrix
add a util function for generating a random linear binary matrix (also useful for the CNOTDihedral class)
change the transpiler pass of LinearFunctionsSynthesis so it will not return a circuit if it has more CX gates and/or worse depth than the original circuit.
add tests and docstrings

qiskit-bot · 2022-08-17T09:48:10Z

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:

@Cryoris
@Qiskit/terra-core
@ajavadia

coveralls · 2022-08-17T10:17:11Z

Pull Request Test Coverage Report for Build 3385868016

122 of 129 (94.57%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 84.542%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/synthesis/high_level_synthesis.py	1	2	50.0%
qiskit/synthesis/linear/linear_circuits_utils.py	44	46	95.65%
qiskit/synthesis/linear/linear_matrix_utils.py	66	70	94.29%

Totals
Change from base Build 3385852241:	0.02%
Covered Lines:	62450
Relevant Lines:	73869

💛 - Coveralls

ajavadia

can you please add a new test that shows #8519 is fixed?

ShellyGarion · 2022-09-15T11:32:59Z

can you please add a new test that shows #8519 is fixed?

Since this PR is already quite long, it's better that this PR will handle only the necessarily linear utils, and we would take care of the transpiler passes in a following PR, preferably after #8548 will get merged.

alexanderivrii

Thanks, @ShellyGarion, LGTM!

kdk

This looks great @ShellyGarion ! A few small questions:

Is there a similar plan to move the AQC code to qiskit/synthesis?
What was the decision on renaming cnot_synth to PMH_cnot_synth?

These functions were in a bit of an unusual state in that they were in some ways internal (not exported by default, did not appear in documentation) and in some ways public (docstring commented, name didn't start with leading _). It'd be nice if they could fall one way or the other. @mtreinish Are there other examples of code in a similar state?

releasenotes/notes/linear_function_synthesis_utils-f2f96924ca45e1fb.yaml

test/python/synthesis/test_linear_synthesis.py

qiskit/synthesis/linear/linear_circuits_utils.py

qiskit/synthesis/linear/linear_matrix_utils.py

ShellyGarion · 2022-11-02T13:57:54Z

Is there a similar plan to move the AQC code to qiskit/synthesis?

What was the decision on renaming cnot_synth to PMH_cnot_synth?

These functions were in a bit of an unusual state in that they were in some ways internal (not exported by default, did not appear in documentation) and in some ways public (docstring commented, name didn't start with leading _). It'd be nice if they could fall one way or the other. @mtreinish Are there other examples of code in a similar state?

We agree that AQC should be moved in the future, but this is not a part of this PR.
We renamed cnot_synth to synth_cnot_count_full_pmh, to indicate that the method optimizes cx gate count, and is for full connectivity. As it's still an internal function, this could be changed in following PRs if needed.
We're not sure if these functions should appear in the API, or only be accessible via the HLS plugin.
For example, in clifford_decompose there are three functions for decomposing clifford circuits for full connectivity:
decompose_clifford_ag, decompose_clifford_bm, decompose_clifford_greedy.

kdk

Thanks for the updates!

This commit reverts a breaking change made in Qiskit#8568, in that PR the graysynth module was moved to the qiskit.synthesis package and the cnot_synth function was also renamed as part of the move. However, this change would break any existing users of the module without any warning. To fix this for the 0.23.0 release this commit adds back the graysynth module and just exports the contents of the module in its new location. Additionally, a small wrapper function is added so that the legacy cnot_synth function name can continue to be used for the time being. In the 0.24.0 cycle we can deprecate the old module and remove them after we've giving users a warning about the change.

One release note to call out is the release note for Qiskit#8568 has been changed significantly. This is based on Qiskit#9445 which is reverting some breaking changes made as part of Qiskit#8568.

* Add back qiskit.transpiler.passes.synthesis.graysynth module This commit reverts a breaking change made in #8568, in that PR the graysynth module was moved to the qiskit.synthesis package and the cnot_synth function was also renamed as part of the move. However, this change would break any existing users of the module without any warning. To fix this for the 0.23.0 release this commit adds back the graysynth module and just exports the contents of the module in its new location. Additionally, a small wrapper function is added so that the legacy cnot_synth function name can continue to be used for the time being. In the 0.24.0 cycle we can deprecate the old module and remove them after we've giving users a warning about the change. * Restore root qiskit.transpiler.synthesis exports * Fix import issues

* Add back qiskit.transpiler.passes.synthesis.graysynth module This commit reverts a breaking change made in #8568, in that PR the graysynth module was moved to the qiskit.synthesis package and the cnot_synth function was also renamed as part of the move. However, this change would break any existing users of the module without any warning. To fix this for the 0.23.0 release this commit adds back the graysynth module and just exports the contents of the module in its new location. Additionally, a small wrapper function is added so that the legacy cnot_synth function name can continue to be used for the time being. In the 0.24.0 cycle we can deprecate the old module and remove them after we've giving users a warning about the change. * Restore root qiskit.transpiler.synthesis exports * Fix import issues (cherry picked from commit bf8d6fa)

…9446) * Add back qiskit.transpiler.passes.synthesis.graysynth module This commit reverts a breaking change made in #8568, in that PR the graysynth module was moved to the qiskit.synthesis package and the cnot_synth function was also renamed as part of the move. However, this change would break any existing users of the module without any warning. To fix this for the 0.23.0 release this commit adds back the graysynth module and just exports the contents of the module in its new location. Additionally, a small wrapper function is added so that the legacy cnot_synth function name can continue to be used for the time being. In the 0.24.0 cycle we can deprecate the old module and remove them after we've giving users a warning about the change. * Restore root qiskit.transpiler.synthesis exports * Fix import issues (cherry picked from commit bf8d6fa) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

* Prepare 0.23.0 release This commit prepares the 0.23.0 release, this involves 2 steps first changing all the version numbers to 0.23.0 from 0.23.0rc1 and secondly updating the release notes to prepare them for publishing. One key thing to note is that this PR removes the 0.22.0 release notes. This is because for the 0.22.0rc1 tag we neglected to move the release notes to a separate directory. So when we did that for the 0.22.0 final release we had to forward port this back to main so that any backports to stable/0.22 would be backportable (see #8901). However, this causes reno to detect all the 0.22 release notes are incorrectly as part of the 0.23.0 development series. To fix this the simplest path forward was to remove the 0.22.0 release notes from the 0.23.0 branch (as in this PR). * Remove backported PRs * Start editting release note * More updates * Apply suggestions from code review Co-authored-by: Julien Gacon <gaconju@gmail.com> * Fix docs build errors * More udpates * Update more release notes One release note to call out is the release note for #8568 has been changed significantly. This is based on #9445 which is reverting some breaking changes made as part of #8568. * More updates * Finish feature note first pass * Start upgrade notes * Move and update new release notes * Fix docs build error * Finish first pass at upgrade notes * Finish first pass at deprecation notes * Finish first pass over release notes * Fix import cycle from dagcircuit * Apply suggestions from code review Co-authored-by: Kevin Hartman <kevin@hart.mn> * Apply suggestions from code review Co-authored-by: Jake Lishman <jake@binhbar.com> * Update releasenotes/notes/0.23/prepare-0.23.0-release-0d954c91143cf9a4.yaml * Fix analysis class definition in vf2 release note Co-authored-by: Julien Gacon <gaconju@gmail.com> Co-authored-by: Kevin Hartman <kevin@hart.mn> Co-authored-by: Jake Lishman <jake@binhbar.com>

move graysynth from transpiler.synthesis to synthesis.linear

7bf50cb

ShellyGarion added the synthesis label Aug 17, 2022

ShellyGarion assigned alexanderivrii Aug 17, 2022

ShellyGarion requested a review from a team as a code owner August 17, 2022 09:48

ShellyGarion added 4 commits August 17, 2022 14:19

add linear utilities for binary matrices: inverse matrix, random matrix

0fc0946

update random invertible binary matrix in random_cnotdihedral

8fa1ba2

update random invertible binary matrix in test_linear_function

61fdce5

fix deprecation warning in graysynth

0309580

ShellyGarion requested a review from ikkoham as a code owner August 17, 2022 11:24

ShellyGarion added 11 commits August 17, 2022 15:09

style

f6a0735

update variable names

b0f7fba

add a function with 4 options to synthesize a CX circuit

48ac9c2

improve linear_utils code

c824d74

Merge branch 'main' into shelly_main_linear

6b5ba6f

refactor transpiler/test_synthesis to synthesis/test_gray_synthesis

ec0043f

split linear_utils into two files

4f97b6c

style

e56f26c

add test/python/synthesis init file

01ffbed

unify linear utils code

aec2006

add check_invertible_binary_matrix function

e8b1121

ajavadia reviewed Sep 15, 2022

View reviewed changes

alexanderivrii mentioned this pull request Sep 19, 2022

Add architecture aware re-synthesis transpilation passes #8769

Closed

ShellyGarion added 5 commits October 19, 2022 11:22

Merge branch 'main' into shelly_main_linear

c52eb98

update import in high_level_synthesis transpiler pass

50b0998

update transpose_cx_circ, add docstrings

e684490

minor fix in calc_inverse_matrix

32dc453

add types in docstrings

760bb2b

alexanderivrii previously approved these changes Oct 25, 2022

View reviewed changes

alexanderivrii added this to the 0.23.0 milestone Oct 30, 2022

kdk reviewed Oct 31, 2022

View reviewed changes

ShellyGarion added 4 commits November 2, 2022 14:51

renmae cnot_synth to PMH_cnot_synth

bac7a0b

add comments following review

636bcdd

fix release notes following review

80c3378

add a test following review

204e98b

ShellyGarion dismissed alexanderivrii’s stale review via 204e98b November 2, 2022 12:57

ShellyGarion added 2 commits November 2, 2022 15:08

rename cnot_synth to PMH_cnot_synth

2140cec

rename to synth_cnot_count_full_pmh

b0c04c2

format tests

a47e03c

kdk previously approved these changes Nov 2, 2022

View reviewed changes

kdk added automerge Changelog: New Feature Include in the "Added" section of the changelog Changelog: API Change Include in the "Changed" section of the changelog Changelog: Bugfix Include in the "Fixed" section of the changelog labels Nov 2, 2022

fix conflict in graysynth.py

01c0bdc

ShellyGarion dismissed kdk’s stale review via 01c0bdc November 3, 2022 07:16

ShellyGarion requested a review from kdk November 3, 2022 08:26

kdk approved these changes Nov 3, 2022

View reviewed changes

Merge branch 'main' into linear_synthesis

1395954

mergify bot merged commit 94bccd5 into Qiskit:main Nov 3, 2022

mtreinish mentioned this pull request Jan 24, 2023

Add back qiskit.transpiler.passes.synthesis.graysynth module #9445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve LinearFunction synthesis #8568

Improve LinearFunction synthesis #8568

ShellyGarion commented Aug 17, 2022 •

edited

Loading

qiskit-bot commented Aug 17, 2022

coveralls commented Aug 17, 2022 •

edited

Loading

ajavadia left a comment

ShellyGarion commented Sep 15, 2022

alexanderivrii left a comment

kdk left a comment

ShellyGarion commented Nov 2, 2022 •

edited

Loading

kdk left a comment

Improve LinearFunction synthesis #8568

Improve LinearFunction synthesis #8568

Conversation

ShellyGarion commented Aug 17, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Aug 17, 2022

coveralls commented Aug 17, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3385868016

💛 - Coveralls

ajavadia left a comment

Choose a reason for hiding this comment

ShellyGarion commented Sep 15, 2022

alexanderivrii left a comment

Choose a reason for hiding this comment

kdk left a comment

Choose a reason for hiding this comment

ShellyGarion commented Nov 2, 2022 • edited Loading

kdk left a comment

Choose a reason for hiding this comment

ShellyGarion commented Aug 17, 2022 •

edited

Loading

coveralls commented Aug 17, 2022 •

edited

Loading

ShellyGarion commented Nov 2, 2022 •

edited

Loading