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

Add qk_obs_apply_layout plus necessary tooling #14106

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Mar 26, 2025

Summary

This PR adds support to apply layouts to QkObs in the C API.

Details and comments

This is done by exposing NLayout to represent the layout, which we also expect to be the result type of routing passes like VF2Layout. It would also be possible to use plain &[u32] but this way qk_obs_apply_layout always uses a consistent type. The layout is exposed as opaque pointer called QkLayout with associated functions qk_layout_new and qk_layout_free.

Cryoris added 2 commits March 26, 2025 16:35
This exposes ``NLayout`` to represent the layout, which we expect to be the result type of routing passes. It would also be possible to use plain &[u32] but this way ``qk_obs_apply_layout`` always uses a consistent type.
@Cryoris Cryoris added Changelog: New Feature Include in the "Added" section of the changelog C API Related to the C API labels Mar 26, 2025
@Cryoris Cryoris requested a review from a team as a code owner March 26, 2025 16:13
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@@ -91,8 +93,8 @@ impl VirtualQubit {
#[pyclass(module = "qiskit._accelerate.nlayout")]
#[derive(Clone, Debug)]
pub struct NLayout {
virt_to_phys: Vec<PhysicalQubit>,
phys_to_virt: Vec<VirtualQubit>,
pub virt_to_phys: Vec<PhysicalQubit>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are made public to be able to easily call SparseObservable::apply_layout, which takes the virt_to_phys vector. This could be worked around by using the available public methods, but I think that would require a copy that I wanted to avoid.


// Cast the slice of PhysicalQubit (which is u32) to plain u32, which is required
// by the apply_layout method.
let as_slice: &[u32] = ::bytemuck::cast_slice(&layout.virt_to_phys);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line (and the NoUninit for PhysicalQubit) warrants extra care since it's the first time I'm using bytemuck 😄

@coveralls
Copy link

coveralls commented Mar 26, 2025

Pull Request Test Coverage Report for Build 14092960259

Details

  • 13 of 54 (24.07%) changed or added relevant lines in 4 files are covered.
  • 11 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 88.047%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/cext/src/sparse_observable.rs 0 16 0.0%
crates/cext/src/layout.rs 0 25 0.0%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.79%
crates/qasm2/src/lex.rs 4 92.98%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 14090658887: -0.04%
Covered Lines: 72762
Relevant Lines: 82640

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Honestly, exposing NLayout like this feels premature to me. The low-level Rust interface to SparseObservable::apply_layout doesn't take NLayout, only layout: Option<&[u32]>, num_qubits: u32, and NLayout doesn't represent everything that TranspileLayout does. Right now, C still has to completely manually build the NLayout, so if anything, exposing NLayout makes it harder, not easier.

I'd suggest we wait til we actually know what we're going to be returning from any form of the transpiler. NLayout might be accessible, but the "high-level" return type from the transpiler representing layouts is still presumably going to be a TranspileLayout-like wrapper. SparseObservable.apply_layout doesn't take a Layout in Python space, which is the analogue.

@Cryoris
Copy link
Contributor Author

Cryoris commented Mar 27, 2025

Just using an array instead of NLayout would also be fine, the idea was to make it very ergonomic to use in a workflow where you'd just call e.g. VF2Layout and feed the output into qk_obs_apply_layout. Since #14056 returns something that is almost NLayout that seemed like a good candidate 😄

But won't we have to introduce equivalents of both Layout and TranspileLayout anyways, since the former is what the layouting passes generate and the latter what we associate with the final circuit?

@jakelishman
Copy link
Member

We'll likely need something akin to TranspileLayout at least. VF2 does produce something like an NLayout, but VF2 isn't the whole story - the layout stage only finds the initial layout of a circuit, but apply_layout on operators depends on both that and any induced routing permutation, the output of VF2 won't necessarily be what we want here.

@jakelishman
Copy link
Member

My point is: we're not at a point yet where we can know what will be ergonomic. Let's not guess - there's no harm in presenting a low-level interface and then waiting a bit til we're more sure to present a high-level one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C API Related to the C API Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants