Refactor ProgramNode::call to flatten+call_flat+unflatten by ihincks · Pull Request #16298 · Qiskit/qiskit

ihincks · 2026-05-28T10:55:28Z

This PR split the ProgramNode trait so implementations only define call_flat(args: &[Tensor]) -> Result<Vec<Tensor>, _> while a new ProgramNodeExt trait provides the DataTree-I/O call(args: &DataTree<Tensor>) on top (which uses the blanket impl of ProgramNode trick to disallow specializations). This comes with three types of enum error classes: (CallInputError, CallError, MissingCallError).

Since Store is the only implementation of ProgramNode that's been merged into main so far, it's the only one that needs to change. However, you can (and should) especially check out QuantumProgram higher in the PR stack to get a sense for how call_flat is actually used: QuantumProgram uses it directly, and gets to totally avoid dealing with hash maps because it can reason about all arguments positionally.

The above ProgramNode refactor is itself straight forward. However, this PR also adds/updates a bunch of supporting DataTree machinery.

PR Stack

Parent: (None, points at main)
Full Stack

AI/LLM disclosure

I didn't use LLM tooling, or only used it privately.
I used the following tool to help write this PR description:
I used the following tool to generate or modify code: claude opus 4-7 and sonnet 4-6

ihincks · 2026-05-28T10:56:46Z

The diff on data_tree.rs is currently too aggressive, I'll come back soon to fix it and make fewer changes.

coveralls · 2026-05-28T11:26:41Z

Coverage Report for CI Build 26904388576

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage decreased (-0.05%) to 87.482%

Details

Coverage decreased (-0.05%) from the base build.
Patch coverage: 30 uncovered changes across 3 files (220 of 250 lines covered, 88.0%).
576 coverage regressions across 13 files.

Uncovered Changes

File	Changed	Covered	%
crates/providers/src/program_node.rs	25	0	0.0%
crates/providers/src/data_tree.rs	202	199	98.51%
crates/providers/src/store.rs	23	21	91.3%

Coverage Regressions

576 previously-covered lines in 13 files lost coverage.

Top 10 Files by Coverage Loss	Lines Losing Coverage	Coverage
crates/circuit/src/operations.rs	179	81.98%
crates/circuit/src/circuit_data.rs	110	86.55%
crates/circuit/src/packed_instruction.rs	64	87.02%
crates/qpy/src/circuit_writer.rs	50	92.79%
crates/qpy/src/py_methods.rs	38	83.21%
crates/transpiler/src/passes/high_level_synthesis.rs	35	86.65%
crates/qpy/src/value.rs	31	71.01%
crates/transpiler/src/passes/basis_translator/compose_transforms.rs	30	74.42%
crates/transpiler/src/passes/basis_translator/mod.rs	16	86.95%
crates/circuit/src/instruction.rs	10	86.87%

Coverage Stats


Relevant Lines:	124513
Covered Lines:	108927
Line Coverage:	87.48%
Coverage Strength:	962192.97 hits per line

💛 - Coveralls

…elpers Split the ProgramNode trait so implementations only define `call_flat(args: &[Tensor]) -> Result<Vec<Tensor>, _>` while a blanket ProgramNodeExt provides the DataTree-shaped `call(args: &DataTree<Tensor>)` on top. Adds the supporting DataTree machinery (`flatten_against`, `unflatten`, `map_leaves`, `into_leaves`, `TreeMatchError`, `ArityMismatch`) and structured call errors (`CallInputError`, `CallError`, `MissingCallError`). Store is rewritten on top of `call_flat` and stores its tensors as a flat `Vec<Tensor>` aligned with `output_types`.

qiskit-bot · 2026-06-01T12:57:11Z

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

@Qiskit/terra-core

mtreinish

I've made it through the data tree changes, but I'm out of time for today to review the rest of of the changes so I'll leave the comments I have so far. Besides the inline comments I'm wondering if we should split this PR up a bit to make it easier to review a lot of the data tree changes in isolation, I think you left a comment this already. I guess it's kind of moot now since I made it through most of the data tree changes already though so probably not worth changing at this point.

I think my biggest concern right now is the change to the iterators to basically collect everything into a vec up front and then iterate over that vec. That kind of eliminates the benefits of using an iterator. If we need that new form for some reason I'd say we should just return the Vec directly and let the user iterate over it. That'll be more explicit about how they work and also give the caller more control on how to use the result.

mtreinish · 2026-06-01T21:17:48Z

+        iter_paths_inner(self, &mut Vec::new(), &mut out);
+        out.into_iter()


I'm not sure how I feel about this change and the other iterators. The previous method was actually an iterator, it was building the output as it was going through the data tree. This changes it so it's not really working as an iterator, it's completely iterating over the entire DataTree building a vec of the expected iterator output and then iterating over that vec as the output.

I'm terribly sorry, but I moved this out of draft mode thinking that I had successfully pushed a change that reverts many unnecessary changes to this file, including this one. The version you have reviewed makes too many out-of-scope changes.

Please excuse the force-push-after-review, but it just seemed cleaner as the correction commit would have been pretty messy. In the new diff, you can see that this PR now touches significantly less stuff. Except in one or two small spots, it only touches things it needs to.

ihincks · 2026-06-02T20:26:08Z

    /// assert_eq!(leaves, expected);
    /// ```
-    pub fn iter_leaves(&self) -> IterLeaves<'_, T> {
+    pub fn iter_leaves(&self) -> impl Iterator<Item = &T> {


I can revert if you'd like this in a different PR, it's not directly related to this one, but I thought it would be nice to take the iterator classes out of the public API of this class so that we're free to update them as needed.

This is fine, I don't think it matters a ton either way. The rust APIs are all private since we don't publish them so we're free to change this at any point. But this is probably easier for people to read when working in the rust code.

mtreinish · 2026-06-02T21:22:18Z

+    ) -> Result<DataTree<Tensor>, CallError<Self::CallError>> {
+        let flat = self
+            .input_types()
+            .flatten_against(args)


Are you worried about the copy this requires for every call? It's not necessarily an issue to start I guess, but my concern is that we'll paint ourselves into a corner in the public interface if we're requiring a flattened input in python and c impl's call_flat() and we can't avoid a copy.

I just worry how large these data trees could get and whether we really need to be copying it all for call, especially as we're going to be passing a slice to call_flat here so the implementor doesn't need an owned copy necessarily.

I should lead with two things that affect the calculus here quite a bit, but have not been merged in yet as they appear later in the PR stack:

QuantumProgram does not call call(), it calls call_flat(), when evaluating the graph. Indeed, the main point of this refactor is to give QuantumProgram that ability. This means that flatten_against is only invoked once on entry, and so the relevant question becomes "how many inputs will a QuantumProgram have?" instead of "how many times will a node be called"? My expectation is usually 0, and typically <100, but I can't predict the future.

The next PR switches from Tensor to ArcTensor, so cloning the list doesn't copy the tensor data, it just increments some counter.

Now given that, we can still switch to call_flat accepting &[&Tensor] (and whatever that implies for flatten_against. Would you like to do that? It doesn't seem like too much work.

I had overlooked the next PR in the chain with Arc so my concern about cloning here is mitigated quite a bit by that. We'll still be allocating a vec of n pointers for each call to call() but that's not terrible. So I'm fine with this given the follow on PRs.

mtreinish

This LGTM now thanks for all the updates on this.

mtreinish · 2026-06-03T19:32:31Z

+        loop {
+            let top = self.stack.last_mut()?;
+            match top.next() {
+                None => {
+                    self.stack.pop();
+                }
+                Some(DataTree::Leaf(v)) => return Some(v),
+                Some(DataTree::Branch(b)) => self.stack.push(b.data.into_iter()),
+            }
+        }
+    }


I was thinking it'd be better to do this with a while loop, but testing this locally you either drop the ? from last_mut() and then add an explicit None return at the end of the function. Or you end up with while let top = self.stack.last_mut() which I assume you tried because clippy told me I should use a loop { ... } with a let inside instead. So I'm fine with this.

This was referenced May 28, 2026

BackendV3 #15902

Open

Switch Tensor to use ArcArray instead of Array #16256

Open

ihincks added mod: providers Related to the backend and job abstractions Rust This PR or issue is related to Rust code in the repository labels May 28, 2026

ihincks added this to the 2.5.0 milestone May 28, 2026

github-project-automation Bot added this to Qiskit 2.5 May 28, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 May 28, 2026

ihincks commented May 28, 2026

View reviewed changes

Comment thread crates/providers/src/data_tree.rs Outdated

ShellyGarion assigned mtreinish May 28, 2026

ihincks commented May 28, 2026

View reviewed changes

Comment thread crates/providers/src/data_tree.rs Outdated

ihincks marked this pull request as ready for review June 1, 2026 12:57

ihincks requested a review from a team as a code owner June 1, 2026 12:57

ihincks requested a review from gadial June 1, 2026 12:57

mtreinish reviewed Jun 1, 2026

View reviewed changes

Fix double iteration in unflatten()

9e03221

ihincks force-pushed the program-node-flat-call branch from c4f4b64 to 9e03221 Compare June 2, 2026 20:21

ihincks commented Jun 2, 2026

View reviewed changes

Comment thread crates/providers/src/data_tree.rs

ihincks requested a review from mtreinish June 2, 2026 20:32

mtreinish reviewed Jun 2, 2026

View reviewed changes

ihincks added 4 commits June 3, 2026 12:15

three small changes from code review

17907d3

inlined helper functions into the functions they help

be7fee3

write a custom iterator struct for into_leaves()

5b1e857

add docstring for IntoLeaves::stack

89f511c

mtreinish added the Changelog: None Do not include in the GitHub Release changelog. label Jun 3, 2026

mtreinish approved these changes Jun 3, 2026

View reviewed changes

mtreinish added this pull request to the merge queue Jun 3, 2026

Merged via the queue into Qiskit:main with commit 005fe52 Jun 3, 2026
48 of 50 checks passed

github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Jun 3, 2026

		iter_paths_inner(self, &mut Vec::new(), &mut out);
		out.into_iter()

Conversation

ihincks commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Stack

AI/LLM disclosure

Uh oh!

ihincks commented May 28, 2026

Uh oh!

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

Uh oh!

Coverage Report for CI Build 26904388576

Coverage decreased (-0.05%) to 87.482%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

qiskit-bot commented Jun 1, 2026

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ihincks commented May 28, 2026 •

edited

Loading

coveralls commented May 28, 2026 •

edited

Loading