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

Fix arguments for RZXCalibrationBuilder and EchoRZXWeylDecomposition #7331

Merged
merged 3 commits into from
Dec 1, 2021

Conversation

mtreinish
Copy link
Member

Summary

The RZXCalibrationBuilder and EchoRZXWeylDecomposition transpiler passes
were previously taking BaseBackend instances as arguments. Besides that
being a deprecated class which will be removed soon, it also is
incorrect because backend objects are not guaranteed to be pickleable so
when a pass manager runs in parallel processes this will cause an error.
This was never caught because these passes aren't part of any default
pass managers and their tests don't include running them as part of
transpile().

To fix this passes typically take the properties of a backend they
require. This is being reworked for BackendV2 in #5885 so a target
object can be used to encapsulate the model of a backend so we have a
single data structure to pass around, but until that is the minimum
backend version we need to also support taking the individual components
for BackendV1 and BaseBackend.

This commit changes the pass constructors to take only use the parameters
from the backend object used in the pass internals and stop requiring a
backend object be used to construct an instance of either pass.

Details and comments

@mtreinish mtreinish added the Changelog: None Do not include in changelog label Dec 1, 2021
@mtreinish mtreinish added this to the 0.19 milestone Dec 1, 2021
The RZXCalibrationBuilder and EchoRZXWeylDecomposition transpiler passes
were previously taking BaseBackend instances as arguments. Besides that
being a deprecated class which will be removed soon, it also is
incorrect because backend objects are not guaranteed to be pickleable so
when a pass manager runs in parallel processes this will cause an error.
This was never caught because these passes aren't part of any default
pass managers and their tests don't include running them as part of
transpile().

To fix this passes typically take the properties of a backend they
require. This is being reworked for BackendV2 in Qiskit#5885 so a target
object can be used to encapsulate the model of a backend so we have a
single data structure to pass around, but until that is the minimum
backend version we need to also support taking the individual components
for BackendV1 and BaseBackend.

This commit changes the pass constructors to take only use the parameters
from the backend object used in the pass internals and stop requiring a
backend object be used to construct an instance of either pass.
@coveralls
Copy link

coveralls commented Dec 1, 2021

Pull Request Test Coverage Report for Build 1526256998

  • 10 of 16 (62.5%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.002%) to 82.898%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/calibration/builders.py 8 14 57.14%
Totals Coverage Status
Change from base Build 1526218462: -0.002%
Covered Lines: 50590
Relevant Lines: 61027

💛 - Coveralls

@mtreinish mtreinish added Changelog: Deprecation Include in "Deprecated" section of changelog and removed Changelog: None Do not include in changelog labels Dec 1, 2021
Copy link
Member

@levbishop levbishop left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot merged commit 6b6c683 into Qiskit:main Dec 1, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants