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

Default set to turn off approximation_degree #8595

Merged
merged 13 commits into from
Sep 16, 2022

Conversation

nonhermitian
Copy link
Contributor

Summary

approximation_degree breaks unitary equivalence of the transpiler. This sets the default to off unless explicitly set.

Details and comments

A related issue (which I have not modified here) is that both approximation_degree=1.0 and approximation_degree=0 turn off the feature due to a bug (per the documentation 1.0 turns it off). However when thinking about "degree(s)" in other contexts, lower usually means less. And therefore 1.0 should actually be maximum approximation rather than none. This also aligns with boolean values. I think the code should be changed to reflect this.

@nonhermitian nonhermitian requested a review from a team as a code owner August 22, 2022 15:08
@mtreinish mtreinish added the Changelog: API Change Include in the "Changed" section of the changelog label Aug 22, 2022
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.

I'm in favor of doing this because I agree this is a bad default. However, before we can merge this it needs a release note. Since this is an API change and users are potentially relying on the previous default (pretty much anyone running QV on hardware for real cares about this) the key thing for this PR is to have a release note that documents the change and provides a thorough explanation of why we're breaking things and how users can update if they desire the old default behavior. The process for adding release notes is here: https://github.com/Qiskit/qiskit-terra/blob/main/CONTRIBUTING.md#release-notes

@nonhermitian
Copy link
Contributor Author

Yes. However I did not want to write a note until the second point about definition of what degree means is resolved. I am happy to make that change here as well.

@kdk
Copy link
Member

kdk commented Aug 30, 2022

Yes. However I did not want to write a note until the second point about definition of what degree means is resolved. I am happy to make that change here as well.

I think saying something like "approximation_degree is being disabled" should be sufficient. (For what it's worth, approximation_degree is defined, albeit loosely, here

https://github.com/Qiskit/qiskit-terra/blob/367ed4e34a3d662ef8ed8f2b91e8823bba1b8ca8/qiskit/compiler/transpiler.py#L178

There's a bug in handling approximation_degree IIRC in the TwoQubitBasisComposer (by this point, basis_fidelity)

https://github.com/Qiskit/qiskit-terra/blob/dbc81a8dcdae9e9294b9058babf5883258daaa5d/qiskit/quantum_info/synthesis/two_qubit_decompose.py#L1087

that causes the transpiler to erroneously treat approximation_degree=0.0 as approximation_degree=None).

@nonhermitian
Copy link
Contributor Author

Is there one kind of release note preferred over others here? I am thinking "other" but open to whatever type people want.

@jakelishman
Copy link
Member

We use "upgrade" (the section heading is called "API changes" in the built notes) to describe things users should be aware of in the new version - that could be a choice?

@nonhermitian
Copy link
Contributor Author

Ok, will change to "upgrade"

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.

This LGTM thanks for adding the release note. I just have some inline suggestions to make some small tweaks to the release note, then I think this should be good to go.

@nonhermitian nonhermitian added this to the 0.22 milestone Sep 15, 2022
jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request Jan 31, 2024
This brings it inline with `transpile`, as was done in Qiskitgh-8595.
github-merge-queue bot pushed a commit that referenced this pull request Feb 1, 2024
…11695)

* Fix `approximation_degree` default in `generate_preset_pass_maanger`

This brings it inline with `transpile`, as was done in gh-8595.

* Correct release note

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>

---------

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants