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

Address c_if, Permutation, and transpile parameter deprecations in Qiskit 1.3 #1482

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

wshanks
Copy link
Collaborator

@wshanks wshanks commented Nov 5, 2024

  • Permutation instances in tomography experiments have been replaced with PermutationGate instances that work the same way for those experiments. The only user facing change should be the avoidance of deprecation warnings in Qiskit 1.3.
  • c_if usage on conditional gates in some tests has been replaced with if_test blocks around those gates.
  • ParallelExperiment now considers a target transpiler option when determining how large a circuit to make when combining subexperiments into a larger circuit. Previously it considered the number of qubits in a coupling_map transpiler option or in the experiment's backend.
  • Use of instruction_durations as a transpile option was replaced with target in one test (instruction_durations was deprecated as a transpiler parameter).
  • Add explicit order="F" argument to cvxpy.vec() calls. This function started issuing a FutureWarning in the latest version of cvxpy about the default order changing from F to C.

The `c_if` usage on conditional gates has been replaced with `if_test`
blocks around those gates.

The `Permutation` instances have been replaced with `PermutationGate`
instances that work the same way for the cases in Experiments.
@wshanks wshanks added the backport stable potential The issue or PR might be minimal and/or import enough to backport to stable label Nov 5, 2024
Additionally, update ParallelExperiment to expand circuits to the number
of qubits in the `Target` when it is passed a transpiler option.
@wshanks wshanks changed the title Address c_if and Permutation deprecations in Qiskit 1.3 Address c_if, Permutation, and transpile parameter deprecations in Qiskit 1.3 Nov 8, 2024
num_qubits = max(num_qubits, 1 + np.max(coupling_map))
elif target is not None:
num_qubits = max(num_qubits, target.num_qubits)
elif self.backend:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can also remove backend_data usage since the V1 model is also deprecated? Or do we want to continue to support backend_data for third-party custom backends based off of the V1 model?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could remove V1 support soon. In this PR, I just wanted to address deprecations in Qiskit 1.3. Removing support would be a bigger, breaking change. At the moment, there is still at least one third party backend provider using BackendV1 but work is in progress to migrate it to BackendV2.

By the way, the main reason I changed this code is because I modified test_t1_parallel_exp_transpile to use a target instead of a coupling_map as a transpile option and the test started failing because the code here was not expanding the circuit based on the size of the target. For completeness, I added expansion based on backend size as well, though generally backends should have coupling maps that have a size that matches num_qubits.

basis_gates=basis_gates,
instruction_durations=instruction_durations,
coupling_map=coupling_map,
target=target,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting target separately from backend (i.e. constructor backend argument) looks weird to me because target is almost a static configuration and not user-provided options. This programming pattern might confuse the users. I would prefer directly extracting target from backend (or backend_data), so that we can always assume a single source of truth for target information. But this is not strong objection. I'd like to hear your thoughts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I agree that this test is weird. It is testing transpile option behavior with an experiment without a backend set. This has been a philosophical difference between some of us -- whether it should be supported to make an abstract experiment or whether backend should become a required input to BaseExperiment.__init__ so that an experiment is always defined in relation to a backend.

The reason I made changes here is that instruction_durations was deprecated in Qiskit 1.3 as an input to transpile with the guidance to use the target instead. So I moved the durations into a Target so that the test would do the same thing. Looking at it again, I see that it does not use the durations any way. I could just delete instruction_durations and leave basis_gates and coupling_map as they were. Actually, I was not sure if basis_gates or coupling_map would be deprecated later as well. I tried asking here and on Slack but did not get an answer. If the goal is to move as much transpilation data into the Target as possible, I don't see why it is needed to keep basis_gates and coupling_map as individual options.

Stepping back, the point of this test is to check that setting a transpile option on a ParallelExperiment results in that option getting used to transpile the subexperiments' circuits. Would you use a different transpile option than target? And would you set a backend? I think making those changes would be more realistic to how experiments are usually performed but I don't know that they are needed for this test. Most of the remaining transpile options relate to features experiments don't use, like circuit optimization. We could make a custom experiment that has redundant 1Q gates and set a higher optimization level to check that the gates get optimized away, but for something like T1 there is not much to do besides change basis gates.

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 Nov 14, 2024

Choose a reason for hiding this comment

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

I was just thinking of using GenericBackendV2 instead of creating an experiment without backend, which looks unusual. I think valid transpiler options should be {optimization_level, scheduling_method, routing_method, etc...} and target should not be included in here. In that sense, I would prefer hard-coding "user-configurable" transpile options and validate input option name like experimental options do. Anyways this is an API break and probably out of the scope of this PR.

Considering the purpose of this PR, current implementation seems good as-is. Probably we can make option more strict in followup, if we still have bandwidth.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds good. With our previous discussions about experiments and transpilation, I think there are few experiments that need transpilation at all besides layout and basis translation. The only one I can think of is QV but that is so specialized that you often want to do the transpilation by hand before the experiment to make sure the circuits have been optimized.

Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a comment

Choose a reason for hiding this comment

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

I think this PR can be merged as-is to address deprecation warnings in 1.3. We may want to be more careful of handling of target in transpile option.

@wshanks wshanks added this pull request to the merge queue Nov 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 14, 2024
cvxpy's vec() function will change the order in which it unravels
matrices in the future so the order needs to be passed explicitly in
order to keep the current behavior.
@wshanks
Copy link
Collaborator Author

wshanks commented Nov 14, 2024

@nkanazawa1989 Merging was blocked because a new cvxpy release just came out that issues a FutureWarning for some of our code. I addressed the warning the way I think it should be addressed. Feel free to double check my work, but I will put this back in the merge queue for now. The vec function which unravels matrices is changing from following F style to C style ordering. I added order="F" to keep the current behavior. I read through the code for a while to confirm that this was the right approach and I am pretty confident. I tried running the tests with order="C" and confirmed that some of them failed (the ones I expected to fail). I was worried that the matrices being raveled were generated by cvxpy and could have their order changed as well in the future but it seems like they are built up out of scipy.sparse matrix operations.

@wshanks wshanks added this pull request to the merge queue Nov 14, 2024
Merged via the queue into qiskit-community:main with commit 3906340 Nov 14, 2024
11 checks passed
mergify bot pushed a commit that referenced this pull request Nov 14, 2024
… in Qiskit 1.3 (#1482)

* `Permutation` instances in tomography experiments have been replaced
with `PermutationGate` instances that work the same way for those
experiments. The only user facing change should be the avoidance of
deprecation warnings in Qiskit 1.3.
* `c_if` usage on conditional gates in some tests has been replaced with
`if_test` blocks around those gates.
* `ParallelExperiment` now considers a `target` transpiler option when
determining how large a circuit to make when combining subexperiments
into a larger circuit. Previously it considered the number of qubits in
a `coupling_map` transpiler option or in the experiment's backend.
* Use of `instruction_durations` as a transpile option was replaced with
`target` in one test (`instruction_durations` was deprecated as a
transpiler parameter).
* Add explicit `order="F"` argument to `cvxpy.vec()` calls. This
function started issuing a `FutureWarning` in the latest version of
cvxpy about the default order changing from F to C.

(cherry picked from commit 3906340)
wshanks added a commit that referenced this pull request Nov 14, 2024
… in Qiskit 1.3 (backport #1482) (#1485)

* `Permutation` instances in tomography experiments have been replaced
with `PermutationGate` instances that work the same way for those
experiments. The only user facing change should be the avoidance of
deprecation warnings in Qiskit 1.3.
* `c_if` usage on conditional gates in some tests has been replaced with
`if_test` blocks around those gates.
* `ParallelExperiment` now considers a `target` transpiler option when
determining how large a circuit to make when combining subexperiments
into a larger circuit. Previously it considered the number of qubits in
a `coupling_map` transpiler option or in the experiment's backend.
* Use of `instruction_durations` as a transpile option was replaced with
`target` in one test (`instruction_durations` was deprecated as a
transpiler parameter).
* Add explicit `order="F"` argument to `cvxpy.vec()` calls. This
function started issuing a `FutureWarning` in the latest version of
cvxpy about the default order changing from F to C.
<hr>This is an automatic backport of pull request #1482 done by
[Mergify](https://mergify.com).

Co-authored-by: Will Shanks <[email protected]>
@wshanks wshanks deleted the c_if-deprecation branch November 14, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport stable potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants