Add a default optimization level to generate_preset_pass_manager

[mtreinish]

Summary

This commit adds a default value to the generate_preset_pass_manager's optimization_level argument. If it's not specified optimization level 2 will be used. After #12148 optimization level 2 is a better fit for an optimal tradeoff between heuristic effort and runtime that makes it well suited as a default optimization level.

Details and comments

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 10114185659

Details

• 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
• 21 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.02%) to 89.72%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	2	88.39%
crates/qasm2/src/lex.rs	7	91.35%
crates/qasm2/src/parse.rs	12	97.15%

Totals
Change from base Build 10111859581:	-0.02%
Covered Lines:	66687
Relevant Lines:	74328

---

💛 - Coveralls

[kevinhartman]

Personally, I'd rather this function not support any positional arguments.

I think we want a more ergonomic API on top of this function eventually anyways, e.g. #12161, so it seems too kludgy to me to try and make this accept a target positionally here.

[mtreinish]

#12161 is actually what I'm not sure we want to do. Adding a 3rd API with different semantics to do what we already have two interfaces for seems like a mistake to me. If people really want a different name I feel like we really should just alias it (but I still don't think it's worth it). But adding yet another entrypoint to accomplish the same thing feels like a mistake. If people are complaining about the ergonomics of the existing interface I feel like we should just evolve it in-place instead of diverging it again and requiring people to learn yet another thing.

[jakelishman]

Yeah, it's unfortunate that we've ended up here, but I think the right thing to do for users is to make it so the common patterns {transpile,generate_preset_pass_manager}({backend,target}, optimization_level=2) work the same in both forms. It ends up in an ugly signature for us, but we can tidy that up in place and potentially fix the signature properly for Qiskit 2.0+.

[jakelishman]

This looks fine in principle to me, I'm just concerned about how we're setting this to have a different default optimisation level to transpile. We already get complaints whenever the interfaces of transpile and this diverge around type inference / whatever, and I guess I'd roughly rather we moved the two in step?

I can live with the split if it was already discussed more in the team, though.

[jakelishman]

Yeah, it's unfortunate that we've ended up here, but I think the right thing to do for users is to make it so the common patterns {transpile,generate_preset_pass_manager}({backend,target}, optimization_level=2) work the same in both forms. It ends up in an ugly signature for us, but we can tidy that up in place and potentially fix the signature properly for Qiskit 2.0+.

[mtreinish]

This looks fine in principle to me, I'm just concerned about how we're setting this to have a different default optimisation level to transpile. We already get complaints whenever the interfaces of transpile and this diverge around type inference / whatever, and I guess I'd roughly rather we moved the two in step?

I can live with the split if it was already discussed more in the team, though.

I updated the transpile() default to level 2 to match in: 34e9ee4 I'm a bit worried this is a breaking api change though. While at a high level the semantics are equivalent as the transpile function compiles the circuit in the same way. There were some implicit behaviors that came with level 1 which don't work in higher optimization levels. The two cases I know about are the implicit trivial layout if it's perfect and working with discrete basis sets. The unit tests flagged these as they were relying on this behavior, I updated those tests to explicitly use level 1 in those cases. I've also documented these edge cases in the release notes. It's arguable whether that's is part of our API contract though, because neither are actually defined behavior, more just implicit expectations/assumptions. However, level 1 being the default was explicitly documented...

[mtreinish]

I'm thinking that if we want to do this for 1.1.0 we should drop 34e9ee4 and save that for 1.2. There seem to be a lot of issues with using level2 as the default in testing. There might be some more implicit assumptions I didn't think of that the tests are flagging. FWIW, every commit I've pushed up I ran the tests locally with and everything passed I assume it's either the presence of optional dependencies or py3.8 vs 3.12 that's causing the failures. But it's making me uneasy about making a potentially breaking change at the last minute before a release.

[jakelishman]

An alternative to moving transpile's default in this PR is to set the default for generate_preset_pass_manager to O1, and then move them both to O2 for Qiskit 2.0? Totally agree that it feels too big a step to move transpile's default at this point in time.

I'd like to see them stay in step at all times, but I can live with it if the decision is to go with O2 for this and move transpile to match it later.

[ElePT]

The PR looks good to me, I went in and fixed a couple of missing tests that were optimization-level dependant, or were using the full pipeline to only test the effect of a single transpiler pass (and that made them optimization-level dependant). Before approving, I want to confirm that we want to move on with the optimization level 2 default in both transpile and generate_ppm (I am going to call it like this from now on) for 1.2. Thoughts? @jakelishman @mtreinish @1ucian0

[mtreinish]

I'm good with doing this for 1.2, if there are no objections from anyone else. I'm personally still a bit apprehensive about the API implications on changing transpile() because of the test changes we needed to make and I'm concerned those are implicit user expectations even if they're not explicitly documented around the behavior of transpile(qc, backend). But I'm willing to fall back on the statement that our stability contract is to our documented interfaces, and these underlying assumptions aren't documented as part of transpile(). Then this can be viewed as being no different than changing an optimization pass to be do more work in the preset pass managers.

I will push up a small tweak to the upgrade release note though to elaborate a bit more on the benefits of the change to transpile() since I just realized it's never mentioned.

[ElePT]

I left a few suggestions for the reno.

[ElePT]

I commited the reno suggestions and will approve the PR, @mtreinish, feel free to add it to the merge queue if you agree with the changes or modify them if you don't.