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

Prepare qiskit/transpiler/graysynth.py for deprecation in next release #9795

Merged
merged 18 commits into from
Apr 17, 2023

Conversation

ShellyGarion
Copy link
Member

@ShellyGarion ShellyGarion commented Mar 15, 2023

Summary

As described in #9445 we should properly deprecate qiskit/transpiler/graysynth.py that currently contains the two functions
cnot_synth and gray_synth.

Details and comments

  • The function cnot_synth is refactored to synth_cnot_count_full_pmh in the file qiskit/synthesis/linear/cnot_synth.py .
  • The function graysynth is refactored to synth_cnot_phase_aam in the file qiskit/synthesis/linear_phase/graysynth.py .

See also #9667

@ShellyGarion ShellyGarion added Changelog: Deprecation Include in "Deprecated" section of changelog synthesis labels Mar 15, 2023
@ShellyGarion ShellyGarion added this to the 0.24.0 milestone Mar 15, 2023
@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 Mar 15, 2023

Pull Request Test Coverage Report for Build 4722520110

  • 59 of 60 (98.33%) changed or added relevant lines in 7 files are covered.
  • 21 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 85.806%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/synthesis/linear/cnot_synth.py 48 49 97.96%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 1 95.37%
crates/qasm2/src/lex.rs 2 90.89%
crates/qasm2/src/parse.rs 18 96.65%
Totals Coverage Status
Change from base Build 4721576551: -0.02%
Covered Lines: 70516
Relevant Lines: 82181

💛 - Coveralls

@Eric-Arellano
Copy link
Collaborator

We'll want to use the new helpers from #9676 (temporarily blocked by #9790)

@ShellyGarion ShellyGarion changed the title [WIP] Deprecate qiskit/transpiler/graysynth.py Deprecate qiskit/transpiler/graysynth.py Mar 30, 2023
Copy link
Collaborator

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

The deprecation decorators look good to me. I didn't review the rest.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

These functions don't look eligible for deprecation to me in Terra 0.24, since this PR is the one that's adding their replacement. In the Qiskit deprecation policy, the rule is that the replacement needs to be in place for a full minor release cycle before a warning can be issued - the reason is so that it's always possible to have valid code for the most recent two minor releases of Terra, rather than requiring all users/downstream packages to depend on the bleeding edge if they want to avoid warnings.

For example, in all Qiskit projects we have warnings treated as errors in our test suite, and several projects test against Terra main as well. If we did deprecations at the same time as the replacement, those projects couldn't have code that supports both the current dev version and the current release version simultaneously.

qiskit/synthesis/linear/__init__.py Show resolved Hide resolved
qiskit/transpiler/synthesis/graysynth.py Outdated Show resolved Hide resolved
@ShellyGarion ShellyGarion changed the title Deprecate qiskit/transpiler/graysynth.py Prepare Deprecate qiskit/transpiler/graysynth.py for deprecation in next release Apr 4, 2023
@ShellyGarion ShellyGarion added Changelog: Bugfix Include in the "Fixed" section of the changelog and removed Changelog: Deprecation Include in "Deprecated" section of changelog labels Apr 4, 2023
@ShellyGarion ShellyGarion changed the title Prepare Deprecate qiskit/transpiler/graysynth.py for deprecation in next release Prepare qiskit/transpiler/graysynth.py for deprecation in next release Apr 4, 2023
@alexanderivrii
Copy link
Contributor

@ShellyGarion has asked me to look at the code organization and the names of the functions. This looks good and consistent to what we did in the past. I have not look at the deprecation-related content.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I pushed up a commit that just adds a comment on the re-export to remind us why it exists and comment that it's eligible for deprecation later. Other than that, I think we're good here now. Thanks Shelly.

@jakelishman jakelishman added this pull request to the merge queue Apr 17, 2023
Merged via the queue into Qiskit:main with commit f630fcd Apr 17, 2023
king-p3nguin pushed a commit to king-p3nguin/qiskit-terra that referenced this pull request May 22, 2023
Qiskit#9795)

* transfer cnot-phase (graysynth) synthesis code to qiskit/synthesis/cnot_phase

* prepare qiskit/transpiler/graysynth.py for deprecation

* update test_gray_synthesis

* add cnot-phase (graysynth) code

* update docstring of cnot_phase_synth

* add deprecation functions

* fix tests following deprecations

* style fix

* add release notes

* remove deprecation functions from graysynth.py

* update tests

* update release notes as a bug fix

* add back graysynth to synthesis/linear

* fix link in releaese notes

* Add note on re-export

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants