Skip to content

Deprecate Unroller pass #10607

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Oct 17, 2023
Merged

Deprecate Unroller pass #10607

merged 18 commits into from
Oct 17, 2023

Conversation

nonhermitian
Copy link
Contributor

@nonhermitian nonhermitian commented Aug 10, 2023 •
edited
Loading

Summary

This PR deprecates the Unroller pass and moves the needed functionality over to a PassManager using UnrollCustomDefinitions and BasisTranslator.

Fixes #10605

Details and comments

The Unroller seems unnecessary given that other passes do the same thing, and Unroller is not used in any of the default passmanagers, and can be replaced in add_control.

@nonhermitian nonhermitian requested a review from a team as a code owner August 10, 2023 18:18
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@1ucian0 1ucian0 added this to the 0.45.0 milestone Aug 31, 2023
@mtreinish mtreinish added the Changelog: Deprecation Include in "Deprecated" section of changelog label Sep 6, 2023
@mtreinish
Copy link
Member

So taking a quick look at the qpy failure the error is because the basis translator is causing the internal definition of the controlled gate in circuit the qpy files saved generated by old releases is (via decompose()):

         ┌─────────┐ ┌──────────┐                                                                       ░ ┌─┐      
   q_0: ─┤ U2(0,π) ├─┤ U1(-π/2) ├─■──────────────────■────────────────■───■──────■──────────────────────░─┤M├──────
        ┌┴─────────┴┐└──────────┘ │                  │                │   │      │        ┌───────────┐ ░ └╥┘┌─┐   
   q_1: ┤ U3(π,0,π) ├─────────────■──────────────────■────────────────■───■──────■────────┤ U3(π,0,π) ├─░──╫─┤M├───
        └───────────┘             │P(π) ┌─────────┐┌─┴─┐┌──────────┐┌─┴─┐ │P(0)  │P(-π/2) └───────────┘ ░  ║ └╥┘┌─┐
   q_2: ──────────────────────────■─────┤ Ry(π/4) ├┤ X ├┤ Ry(-π/4) ├┤ X ├─■──────■──────────────────────░──╫──╫─┤M├
                                        └─────────┘└───┘└──────────┘└───┘                               ░  ║  ║ └╥┘
meas: 3/═══════════════════════════════════════════════════════════════════════════════════════════════════╩══╩══╩═
                                                                                                           0  1  2

but with this PR applied the definition is:

         ┌─────────┐ ┌──────────┐                                                                                                                                                                 ░ ┌─┐      
   q_0: ─┤ U2(0,π) ├─┤ U1(-π/2) ├─■──────────────────■────────────────■───■───────■─────────────────────────────────────────────────■────────────────────────────────■────────────────────────────░─┤M├──────
        ┌┴─────────┴┐└──────────┘ │                  │                │   │       │                                                 │                                │P(7π/4)       ┌───────────┐ ░ └╥┘┌─┐   
   q_1: ┤ U3(π,0,π) ├─────────────■──────────────────■────────────────■───■───────┼──────────────────────■──────────────────────────┼──────────────────────■─────────■──────────────┤ U3(π,0,π) ├─░──╫─┤M├───
        └───────────┘             │P(π) ┌─────────┐┌─┴─┐┌──────────┐┌─┴─┐ │P(0) ┌─┴─┐┌────────────────┐┌─┴─┐┌────────────────────┐┌─┴─┐┌────────────────┐┌─┴─┐┌────────────────────┐└───────────┘ ░  ║ └╥┘┌─┐
   q_2: ──────────────────────────■─────┤ Ry(π/4) ├┤ X ├┤ Ry(-π/4) ├┤ X ├─■─────┤ X ├┤ U(0,π/16,π/16) ├┤ X ├┤ U(0,15π/16,15π/16) ├┤ X ├┤ U(0,π/16,π/16) ├┤ X ├┤ U(0,15π/16,15π/16) ├──────────────░──╫──╫─┤M├
                                        └─────────┘└───┘└──────────┘└───┘       └───┘└────────────────┘└───┘└────────────────────┘└───┘└────────────────┘└───┘└────────────────────┘              ░  ║  ║ └╥┘
meas: 3/═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════╩══╩══╩═
                                                                                                                                                                                                     0  1  2

which I think is equivalent but I didn't test it yet. To fix this we need to change the test assertion here to just check that they're unitary equivalent because running the same calls to build a circuit will no longer create an identical circuit when gate.control() is used. I'll push up a quick commit to adjust the tests and also update the release notes to document this as an upgrade note.

@mtreinish
Adjust qpy backwards compat tests to handle change in control definition
e6bc910 
In this PR by switching the internal use of the Unroller to the
BasisTranslator for the add_control.py module results in a different
definition for the generated controlled gate than was done in previous
releases this causes the qpy backwards compat tests to fail. The qpy
backwards compatibility tests generate qpy files with every historical
release of qiskit (that had qpy support) and valdiates that if we load
those qpy files with the current state of the main branch (plus a PR
under test) that the loaded  circuits are identical to running the same
code to generate the circuit. However, because of this divergence due to
an internal change in add_control() the circuits are no longer 100%
identical, but are still equivalent. To address this, a new option is
added to the test suite to give an option when this situation occurs to
test for unitary equivalence instead of a instruction for instruction
identical circuit. This is then used for the control circuits.
@mtreinish
Expand release notes
9f4306d
@mtreinish
Ensure we are explicitly warning on transpile()
24e94f5 
This commit adds an explicit user facing warning for places where the
transpiler is typically invoked by users. The normal entry point for
users that are using the unroller is to call
`transpile(..., translation_method="unroller")` and they don't work
directly with the `Unroller` pass. This means the deprecation warning
would be hidden from most users because the warning would not point to
user code and the default filters will suppress it. This commit adds an
explicit warning to transpile() and generate_preset_pass_manager() when
the unroller is requested as the translation method to ensure users see
the warning and know how to adjust.
@mtreinish mtreinish added the Changelog: API Change Include in the "Changed" section of the changelog label Oct 13, 2023
@ElePT ElePT self-assigned this Oct 16, 2023
ElePT
ElePT reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM, I just have a couple of minor suggestions.

@mtreinish @ElePT
Apply suggestions from code review
a275fff 
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
@alexanderivrii
Copy link
Member

Hmm, I am wondering if in view of #10965, we eventually want to replace Unroller by HighLevelSynthesis?

@mtreinish
Copy link
Member

Hmm, I am wondering if in view of #10965, we eventually want to replace Unroller by HighLevelSynthesis?

I think it would technically be fairly easily to implement after #10965 since it's just changing the recursive exit condition to be if gate in target instead of the equivalence library component too. But in practice I'm not sure how much value it provides. In general we're deprecating the Unroller here because there is limited utility in using it as a method of basis translation because the default definitions for gates are never guaranteed to be in the target basis. So unless you're targeting a backend that's using ['cx', 'u'] using the recursive descent tree like the unroller for synthesis will mostly likely just error.

ElePT
ElePT approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

LGTM!

@mtreinish mtreinish enabled auto-merge October 17, 2023 14:53
@mtreinish mtreinish added this pull request to the merge queue Oct 17, 2023
Merged via the queue into Qiskit:main with commit 881fa8e Oct 17, 2023
This was referenced Oct 20, 2023
Closed
Closed
@nonhermitian nonhermitian mentioned this pull request Mar 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElePT ElePT ElePT approved these changes

Assignees

@mtreinish mtreinish

@ElePT ElePT

Labels
Changelog: API Change Include in the "Changed" section of the changelog Changelog: Deprecation Include in "Deprecated" section of changelog
Projects
None yet
Milestone
0.45.0
Development

Successfully merging this pull request may close these issues.

Remove print statement in Unroller
6 participants
@nonhermitian @qiskit-bot @mtreinish @alexanderivrii @ElePT @1ucian0