Add `qk_circuit_unitary` #14334

maximizemaxwell · 2025-05-09T03:01:55Z

Add C API for unitary gate

Part of issue 14238

#14238

What does this PR do?

Add new function of unitary gate in crates/cext/src/circuit.rs.

Who can review?

@mtreinish @eliarbel @Cryoris

qiskit-bot · 2025-05-09T03:02:02Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

@Qiskit/terra-core

CLAassistant · 2025-05-09T03:02:02Z

All committers have signed the CLA.

coveralls · 2025-05-09T09:58:16Z

Pull Request Test Coverage Report for Build 15296975102

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

39 of 56 (69.64%) changed or added relevant lines in 1 file are covered.
677 unchanged lines in 24 files lost coverage.
Overall coverage decreased (-0.5%) to 87.822%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/cext/src/circuit.rs	39	56	69.64%

Files with Coverage Reduction	New Missed Lines	%
crates/quantum_info/src/convert_2q_block_matrix.rs	1	92.65%
crates/transpiler/src/passes/optimize_1q_gates_decomposition.rs	1	90.8%
qiskit/circuit/library/generalized_gates/gms.py	2	94.44%
crates/quantum_info/src/versor_u2.rs	3	81.85%
qiskit/circuit/library/generalized_gates/diagonal.py	3	95.16%
crates/qasm2/src/lex.rs	4	91.23%
qiskit/circuit/library/generalized_gates/permutation.py	4	92.73%
qiskit/circuit/library/arithmetic/piecewise_linear_pauli_rotations.py	5	92.47%
qiskit/circuit/library/blueprintcircuit.py	7	94.31%
qiskit/qasm3/init.py	7	85.45%

Totals
Change from base Build 15216054136:	-0.5%
Covered Lines:	79864
Relevant Lines:	90939

💛 - Coveralls

mtreinish

Thanks for doing this it's a great start! I left a few inline comments to start. The other thing which will be important here is to add some unit tests to make sure we exercise the new code path. You can add the tests to this file: https://github.com/Qiskit/qiskit/blob/main/test/c/test_circuit.c

crates/cext/src/circuit.rs

mtreinish · 2025-05-09T10:32:10Z

crates/cext/src/circuit.rs

@@ -378,6 +381,59 @@ pub struct OpCounts {
    len: usize,
 }

+/// @ingroup QkCircuit
+/// Append an arbitrary unitary matrix to the circuit.
+/// The user passes a row-major array of interleaved real-imaginary pairs #[


Were you going to link to something here?

mtreinish · 2025-05-09T10:40:38Z

crates/cext/src/circuit.rs

+    let raw = unsafe { std::slice::from_raw_parts(matrix, dim * dim * 2) };
+    let data: Vec<Complex64> = raw
+        .chunks_exact(2)
+        .map(|x| Complex64::new(x[0], x[1]))
+        .collect();
+
+    // Build ndarray::Array2
+    let mat = Array2::from_shape_vec((dim, dim), data).expect("Invalid shape for unitary matrix");


I wonder if using ArrayView::from_shape_ptr() here to create a view of the C array and then doing to_owned() to create a copy of it would be better. I guess the advantage of doing the ArrayView path is that it lets you control the striding too and support various memory layouts. While doing it like this explicit memory layout for what we store in Rust for the UnitaryGate. TBH, I'm fine either way as long as we document it, but I wanted to raise it as a question to discuss at least.

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

maximizemaxwell · 2025-05-13T07:05:06Z

Could u help me writing test codes?

Cryoris · 2025-05-13T07:24:02Z

Sure, we can help push this over the finish line 👍🏻 I had one other question; is there a reason you picked pairs of double instead of just complex numbers? We can use QkComplex64 which represents a complex double.

maximizemaxwell · 2025-05-13T08:16:30Z

No, it can be removed and it would be better to replace it with QkComplex64..

mtreinish

This LGTM now, just some small nits inline. I'll apply the suggestions and then approve this.

crates/cext/src/circuit.rs

mtreinish

Thanks for all the work on this!

This reverts commit 25a34b0.

This broke the C API tests but somehow made it through CI. We need teh CI fix, and to send the PR back to review. This reverts commit 25a34b0.

jakelishman · 2025-05-28T17:20:55Z

@maximizemaxwell: I had to temporarily revert this PR because it was incompatible with #14340 that happened to merge right before it, and our CI didn't stop it from merging to main. It wasn't your fault - you couldn't possibly have known (it wasn't even on main), and it's our fault that CI merged it anyway.

Would you like to open a new PR from the same branch, and finalise the fixes that are needed to make it compatible?

We originally merged this C API function in Qiskit#14334 but it had to be reverted in Qiskit#14492 because the C API tests conflicted with Qiskit#14340 and failed the tests. It was only able to merge because of a brief configuration issue with CI. This commit reapplies the originally PR and fixes the test conflict. This reverts commit 49874c6.

maximizemaxwell · 2025-05-28T22:53:13Z

Sure I'll check and finish soooon

mergify · 2025-05-29T00:19:30Z

⚠️ The sha of the head commit of this PR conflicts with #14499. Mergify cannot evaluate rules on this PR. ⚠️

maximizemaxwell · 2025-05-29T04:33:03Z

Has this issue been resolved?

Cryoris · 2025-05-29T07:45:00Z

You have to open a new PR and update the branch from main, to pull in the latest changes. What you started in #14499 looked correct, you just need to update the branch and update the code to use the new QkComplex64 format for complex numbers. I can help / take over with that, if you need any help 🙂

We originally merged this C API function in Qiskit#14334 but it had to be reverted in Qiskit#14492 because the C API tests conflicted with Qiskit#14340 and failed the tests. It was only able to merge because of a brief configuration issue with CI. This commit reapplies the originally PR and fixes the test conflict. This reverts commit 49874c6.

* Reapply "Add ``qk_circuit_unitary`` (#14334)" (#14492) We originally merged this C API function in #14334 but it had to be reverted in #14492 because the C API tests conflicted with #14340 and failed the tests. It was only able to merge because of a brief configuration issue with CI. This commit reapplies the originally PR and fixes the test conflict. This reverts commit 49874c6. * Add tests for different sized matrices

* add initial qk_circuit_unitary * add dependencies * add dependencies in Cargo.toml * changed circuit pushing logic * fixed unsafe pointer * fixed compile error * Update circuit.rs Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Update circuit.rs Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Update crates/cext/src/circuit.rs Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * initial version of test code * changed order, patterns * replaced double to QkComplex64 * accomodate windows and ubuntu nits * clippppppppppy * add check_input * Use nalgebra for 1/2q cases * fix docs * changed to iter * fixed docs * Apply suggestions from code review * Fix missing /// in docstring --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>

This broke the C API tests but somehow made it through CI. We need teh CI fix, and to send the PR back to review. This reverts commit 25a34b0.

…iskit#14509) * Reapply "Add ``qk_circuit_unitary`` (Qiskit#14334)" (Qiskit#14492) We originally merged this C API function in Qiskit#14334 but it had to be reverted in Qiskit#14492 because the C API tests conflicted with Qiskit#14340 and failed the tests. It was only able to merge because of a brief configuration issue with CI. This commit reapplies the originally PR and fixes the test conflict. This reverts commit 49874c6. * Add tests for different sized matrices

maximizemaxwell added 6 commits May 8, 2025 13:34

add initial qk_circuit_unitary

ab56741

add dependencies

80bd545

add dependencies in Cargo.toml

931396a

changed circuit pushing logic

0e3d48a

fixed unsafe pointer

26fa237

fixed compile error

462f843

maximizemaxwell requested a review from a team as a code owner May 9, 2025 03:01

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label May 9, 2025

github-project-automation bot added this to Contributor Monitoring May 9, 2025

Merge branch 'main' into add_C_API_for_UnitaryGate

dc5b4a3

mtreinish reviewed May 9, 2025

View reviewed changes

mtreinish moved this to PR open / Contributor working on it in Contributor Monitoring May 9, 2025

mtreinish added this to the 2.1.0 milestone May 9, 2025

mtreinish added Changelog: None Do not include in changelog C API Related to the C API labels May 9, 2025

mtreinish assigned mtreinish and Cryoris May 9, 2025

maximizemaxwell and others added 7 commits May 9, 2025 21:46

Update circuit.rs

5457e3d

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

Update circuit.rs

899dce2

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

Merge branch 'main' into add_C_API_for_UnitaryGate

b5ba133

Merge branch 'main' into add_C_API_for_UnitaryGate

e424329

Update crates/cext/src/circuit.rs

d790883

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

initial version of test code

3eee80c

changed order, patterns

4c78cb5

maximizemaxwell added 2 commits May 28, 2025 18:39

changed to iter

4ea1fa7

fixed docs

05ab168

mtreinish reviewed May 28, 2025

View reviewed changes

Apply suggestions from code review

3e5141e

mtreinish previously approved these changes May 28, 2025

View reviewed changes

mtreinish enabled auto-merge May 28, 2025 14:21

Fix missing /// in docstring

0847ed9

mtreinish dismissed their stale review via 0847ed9 May 28, 2025 15:02

mtreinish approved these changes May 28, 2025

View reviewed changes

mtreinish added this pull request to the merge queue May 28, 2025

Merged via the queue into Qiskit:main with commit 25a34b0 May 28, 2025
20 of 26 checks passed

github-project-automation bot moved this from PR open / Contributor working on it to Done in Contributor Monitoring May 28, 2025

jakelishman added a commit to jakelishman/qiskit-terra that referenced this pull request May 28, 2025

Revert "Add qk_circuit_unitary (Qiskit#14334)"

f8340f3

This reverts commit 25a34b0.

jakelishman mentioned this pull request May 28, 2025

Add C API interface for adding a UnitaryGate to a circuit #14238

Closed

github-merge-queue bot pushed a commit that referenced this pull request May 28, 2025

Revert "Add qk_circuit_unitary (#14334)" (#14492)

49874c6

This broke the C API tests but somehow made it through CI. We need teh CI fix, and to send the PR back to review. This reverts commit 25a34b0.

maximizemaxwell mentioned this pull request May 29, 2025

Add qk_circuit_unitary fixes #14499

Closed

2 tasks

mtreinish mentioned this pull request May 30, 2025

Reapply "Add qk_circuit_unitary (#14334)" (#14492) #14509

Merged

Add qk_circuit_unitary #14334

Add qk_circuit_unitary #14334

Uh oh!

Conversation

maximizemaxwell commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add C API for unitary gate

Part of issue 14238

What does this PR do?

Who can review?

Uh oh!

qiskit-bot commented May 9, 2025

Uh oh!

CLAassistant commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15296975102

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish May 9, 2025

Choose a reason for hiding this comment

Uh oh!

mtreinish May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximizemaxwell commented May 13, 2025

Uh oh!

Cryoris commented May 13, 2025

Uh oh!

maximizemaxwell commented May 13, 2025

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakelishman commented May 28, 2025

Uh oh!

maximizemaxwell commented May 28, 2025

Uh oh!

mergify bot commented May 29, 2025

Uh oh!

maximizemaxwell commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cryoris commented May 29, 2025

Uh oh!

Uh oh!

Add `qk_circuit_unitary` #14334

Add `qk_circuit_unitary` #14334

maximizemaxwell commented May 9, 2025 •

edited

Loading

CLAassistant commented May 9, 2025 •

edited

Loading

coveralls commented May 9, 2025 •

edited

Loading

mtreinish May 9, 2025 •

edited

Loading

maximizemaxwell commented May 29, 2025 •

edited

Loading