Skip to content
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

Deprecate LinearFunctionsSynthesis transpiler pass #9041

Merged
merged 10 commits into from
Jan 17, 2023

Conversation

alexanderivrii
Copy link
Contributor

@alexanderivrii alexanderivrii commented Oct 31, 2022

Summary

We already have a high-level-synthesis transpiler pass that is able to synthesize linear functions using the plugin interface. The original LinearFunctionsSynthesis transpiler pass is no longer needed, and we have decided to deprecate it.

In addition, LinearFunctionsSynthesis was previously based on the definition method provided in LinearFunction (which calls the specific cnot_synth algorithm). This may lead to confusion, as it does not use the synthesis plugin interface specified underlinear_function.default in entry_points.

This PR starts the deprecation clock on LinearFunctionsSynthesis, and temporarily reimplements as a special case of HighLevelSynthesis (using the synthesis method specified underlinear_function.default in entry_points).


Additionally comments:

  • The LinearFunctionsSynthesis is not used in the transpilation flow, as transpile calls HighLevelSynthesis and not LinearFunctionsSynthesis.

  • Currently LinearFunction inherits from Gate (which eventually inherits from Operation). We have decided that it's fine to keep it this way.

@alexanderivrii alexanderivrii requested a review from a team as a code owner October 31, 2022 16:31
@qiskit-bot
Copy link
Collaborator

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:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Oct 31, 2022

Pull Request Test Coverage Report for Build 3943107070

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.09%) to 84.859%

Files with Coverage Reduction New Missed Lines %
src/sabre_swap/mod.rs 2 99.54%
src/vf2_layout.rs 3 94.74%
Totals Coverage Status
Change from base Build 3942236674: 0.09%
Covered Lines: 65800
Relevant Lines: 77540

💛 - Coveralls

@alexanderivrii alexanderivrii changed the title reimplementing LinearFunctionsSynthesis as a special case of HighLevelSynthesis Deprecate LinearFunctionsSynthesis transpiler pass Nov 2, 2022
@alexanderivrii alexanderivrii added this to the 0.23.0 milestone Nov 2, 2022
@alexanderivrii alexanderivrii added the mod: transpiler Issues and PRs related to Transpiler label Nov 2, 2022
ShellyGarion
ShellyGarion previously approved these changes Nov 23, 2022
@ShellyGarion ShellyGarion added the Changelog: Deprecation Include in "Deprecated" section of changelog label Nov 23, 2022
@mtreinish mtreinish self-assigned this Nov 29, 2022
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
@@ -20,7 +20,7 @@
from qiskit.circuit import QuantumCircuit, Qubit, Clbit
from qiskit.transpiler.passes.optimization import CollectLinearFunctions
from qiskit.transpiler.passes.synthesis import (
LinearFunctionsSynthesis,
HighLevelSynthesis,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maintain some tests of the old pass to ensure it works as is during the deprecation period.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a test that the old functionality still works and a bug fix (shame on me, I did break the functionality), see 0309abe. I believe that a single test suffices, since most of the tests actually check the orhogonal block collection aspect of resynthesizing linear functions.

There was also a suggestion to extend the @combine(do_commutative_analysis=[False, True]) to something like@combine(do_commutative_analysis=[False, True], synthesis_pass=[HighLevelSynthesis, LinearFunctionsSynthesis]), however I did not end up using this since in one case we want to call

synthesized_circuit = PassManager(synthesis_pass()).run(optimized_circuit)

and in the other case we want to call

with self.assertWarns(DeprecationWarning):
    synthesized_circuit = PassManager(synthesis_pass()).run(optimized_circuit)

and I did not think of a clever way to nicely combine the two.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding the test

@mergify mergify bot merged commit bce6e9a into Qiskit:main Jan 17, 2023
@alexanderivrii alexanderivrii deleted the fix-linear-function-synthesis branch January 20, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Deprecation Include in "Deprecated" section of changelog mod: transpiler Issues and PRs related to Transpiler priority: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants