Skip to content

Add type-safe VecMap for slices indexed by newtypes#14910

Merged
alexanderivrii merged 2 commits intoQiskit:mainfrom
jakelishman:sabre/layers/2
Apr 1, 2026
Merged

Add type-safe VecMap for slices indexed by newtypes#14910
alexanderivrii merged 2 commits intoQiskit:mainfrom
jakelishman:sabre/layers/2

Conversation

@jakelishman
Copy link
Copy Markdown
Member

Sabre uses several objects that are logically maps from an index-like newtype (like NodeIndex or PhysicalQubit) to some value, and are implemented as fixed-slice Vecs for lookup efficiency. The newtype provides type safety while it's in use, but we have to cast it away to index, which makes it easy to index slices with the wrong object.

This introduces a VecMap object, which provides a (minimal) slice-like interface, but indexes using the relevant newtype.

The current implementation of Sabre does not use this too much, but a refactoring of the layer structures will have them store one slice indexed by PhysicalQubit and one by VirtualQubit, which are trivially easy to get switched (a frequent mistake that is the base reason those new types were introduced in the first place).

@jakelishman jakelishman requested a review from a team as a code owner August 14, 2025 15:10
@jakelishman jakelishman added this to the 2.2.0 milestone Aug 14, 2025
@jakelishman jakelishman added the mod: transpiler Issues and PRs related to Transpiler label Aug 14, 2025
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 16969142707

Details

  • 151 of 160 (94.38%) changed or added relevant lines in 6 files are covered.
  • 27 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.006%) to 88.252%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/transpiler/src/passes/sabre/route.rs 112 115 97.39%
crates/circuit/src/nlayout.rs 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/parameter/parameter_expression.rs 1 83.39%
crates/transpiler/src/passes/sabre/dag.rs 1 96.97%
crates/transpiler/src/passes/sabre/route.rs 3 93.93%
crates/qasm2/src/lex.rs 4 92.01%
crates/circuit/src/parameter/symbol_expr.rs 6 74.5%
crates/qasm2/src/parse.rs 12 97.09%
Totals Coverage Status
Change from base Build 16967470304: -0.006%
Covered Lines: 87850
Relevant Lines: 99544

💛 - Coveralls

@raynelfss raynelfss modified the milestones: 2.2.0, 2.3.0 Aug 27, 2025
@github-project-automation github-project-automation Bot moved this to Ready in Qiskit 2.3 Oct 7, 2025
@Cryoris Cryoris removed this from Qiskit 2.3 Dec 12, 2025
@Cryoris Cryoris modified the milestones: 2.3.0, 2.4.0 Dec 12, 2025
@jakelishman jakelishman force-pushed the sabre/layers/2 branch 2 times, most recently from 0fefe86 to 1c4ccc7 Compare March 11, 2026 21:51
@mtreinish mtreinish modified the milestones: 2.4.0, 2.5.0 Mar 19, 2026
@jakelishman jakelishman force-pushed the sabre/layers/2 branch 2 times, most recently from ef46cd4 to 27e782b Compare March 29, 2026 11:10
@jakelishman jakelishman removed the on hold Can not fix yet label Mar 29, 2026
@jakelishman
Copy link
Copy Markdown
Member Author

Sasha: this is next (with #15897). I think, after this, I have to look again in more detail at the next two PRs because it seems like one of them introduces a non-determinism or a platform dependency.

This one is fine, though (without it, I for sure would have made mistakes in writing #14911).

There'll be a minor merge conflict with #15897 - the solution will be to take the hunk from #15897 and ignore this PR.

There is an edge case in release-valve handling where, if the shortest
path is a single swap (but the heuristics have been chosen such that it
is unselectable normally), it is possible for the release valve to cause
_two_ gates to be routed.  The previous handling of this path was
written in quite a complicated manner, likely to be able to use
`closest_node` by name from its discovery earlier in the release-valve
process.

Instead, it is easier to recognise that, given a single swap:

* the two qubits cannot be part of the same gate, or the gate would have
  been routable _without_ the swap;
* the `closest_node` must touch one of these qubits because of the
  the Dijkstra search;
* therefore we can simply add any routable gate that touches _either_
  qubit without risk of duplication or forgetting `closest_swap`.
Copy link
Copy Markdown
Member

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

This is a cool PR!

So basically previously we could write code like

my_data: Vec<f64> // should be indexed by physical qubits
v: VirtualQubit
my_data[v.index()] = ... 

and it would compile, but now we would write

my_data: VecMap<PhysicalQubit, f64> 
v: VirtualQubit
my_data[v] = ... 

and it would not compile (preventing the error).

My only question is whether the new VecMap struct should be exposed to more of Qiskit and not local to sabre?

Comment thread crates/transpiler/src/passes/sabre/route.rs Outdated
@jakelishman
Copy link
Copy Markdown
Member Author

My only question is whether the new VecMap struct should be exposed to more of Qiskit and not local to sabre?

It could be. If you prefer, I can move it into the root of transpiler?

@alexanderivrii
Copy link
Copy Markdown
Member

It could be. If you prefer, I can move it into the root of transpiler?

We don't need to move this for this PR at all, but would this be a natural fit for the new utils crate that you have added?

@jakelishman
Copy link
Copy Markdown
Member Author

The awkward bit about putting it in utils is that it depends on petgraph's IndexType trait right now, and it'd be a bit weird to put a dependency on petgraph into the utils crate. It probably wouldn't have any meaningful impact, since circuit depends on petgraph anyway, but it still feels odd.

Copy link
Copy Markdown
Member

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks!

@alexanderivrii alexanderivrii added this pull request to the merge queue Apr 1, 2026
Merged via the queue into Qiskit:main with commit c33a092 Apr 1, 2026
26 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.4 Apr 1, 2026
@jakelishman jakelishman deleted the sabre/layers/2 branch April 1, 2026 10:33
@ShellyGarion ShellyGarion added the Changelog: None Do not include in the GitHub Release changelog. label Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. mod: transpiler Issues and PRs related to Transpiler

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants