feat!: defgate as sequence #460

erichulburd · 2025-07-13T03:23:13Z

Closes #447

New Features

Parsing and construction of sequence gate definitions.
Expansion of sequence gate definitions with ability to filter by gate name.
Query a source map for sequence gate definition expansion.

Refactor

Refactor Program::expand_calibrations_with_source_map to return SourceMap<InstructionIndex, InstructionTarget<CalibrationExpansion>>.
Refactor Python source map to a single type, shared across calibration and sequence gate definition expansion. Namely, we use enum InstructionTargetShim to cover all known instruction targets and return InstructionSourceMap = SourceMap<std::ops::Range<usize>, InstructionTargetSim>.

Notes on approach

There's a lot of ways to slice this pie, but the goals I decided on were to:

Expose only a single source map to Python.
Support a source map structure in quil-py that can be extended to support Python constructed source maps without a breaking change (or at least none other than handling an additional InstructionTargetShim variant.
Support a source map structure in quil-py that is forward compatible with passes that excise instructions or operate on multiple source instructions (accomplished by using a SourceIndex type of std::ops::Range<usize>).

github-actions · 2025-07-13T03:25:09Z

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-11-06 00:01 UTC

windsurf-bot

Other comments (7)

quil-rs/src/parser/common.rs (274-274) There's an issue with the error message in `parse_argument_qubit`. The code is using `stringify!($expected_variant)` but there's no macro parameter named `$expected_variant` defined in this context. This will cause a compilation error.
```
            expected_token!(input, other_token, "an identifier".to_owned())
```
quil-rs/src/instruction/gate_sequence.rs (87-87) The call to `Gate::new()` uses `unwrap()` which could cause a panic if the gate creation fails. Consider handling this error case explicitly.
quil-rs/src/program/mod.rs (343-343) There's a FIXME comment about dropping expanded gate definitions. Consider addressing this before finalizing the PR to ensure proper cleanup of gate definitions that have been expanded.
quil-rs/src/program/defgate_sequence_expansion.rs (117-117) The `gate_sequence_from_instruction` function takes a mutable reference to `seen` but doesn't modify it. Consider changing the parameter to an immutable reference for clarity.
quil-rs/src/program/defgate_sequence_expansion.rs (80-82) In the recursive calls to expansion functions, you're cloning `seen` and then passing a mutable reference to the clone. This approach is inconsistent with how `seen` is used in the parent function. Consider either passing the original `seen` (if you want changes to propagate up) or using the clone directly without passing it as mutable (if you want isolation).
quil-rs/src/instruction/gate_sequence.rs (25-25) There's a typo in the error variant name: `GateQubitArugment` should be `GateQubitArgument`.
quil-rs/src/program/defgate_sequence_expansion.rs (130-130) There's a typo in the variable name - it has an equals sign attached: `source=`. This should be `source =`.

💡 To request another review, post a new comment with "/windsurf-review".

quil-rs/src/filter_set.rs

quil-rs/src/instruction/gate_sequence.rs

quil-rs/src/program/mod.rs

antalsz

The biggest thing I find lacking from this PR is guidance on how it's going to be entered. Can you write up an example in a doc comment somewhere showing how you'd perform full program expansion, soup to nuts?

The other thing I find surprising is that there's no "just expand everything" option. There are two almost-identical expansion operations which return completely disjoint source map types, when I'd expect you'd mostly just want to do all the expansion at once. It feels like it would be possible to unify those operations and provide an expand function, which would avoid the need to merge source maps.

The problem, of course, is that it's ambiguous which operation should "win" if there are conflicting DEFGATE AS SEQUENCE and DEFCALs. I can see four approaches:

Error if a single instruction could be expanded with both a SEQUENCE and a DEFCAL.
Provide a way of configuring what should happen if there's an ambiguity: prefer the SEQUENCE, prefer the DEFCAL, or error.
As the former, but per-gate.
Provide exactly one resolution b behavior – probably "prefer the SEQUENCE". Document it.

Those are arranged in order of preference.

quil-py/quil/instructions/__init__.pyi

quil-py/quil/program/__init__.pyi

quil-py/src/program/source_map.rs

erichulburd · 2025-08-06T23:20:52Z

Can you write up an example in a doc comment somewhere showing how you'd perform full program expansion, soup to nuts?

The motivation for this work is to define a cycle's calibration and unitary definition within the same program. In this context, calibration and gate definition expansion are orthogonal to each other:

DECLARE params REAL[2]

DEFCAL SOME_CYCLE(%theta, %phi) 0:
  # ...

DEFGATE SOME_CYCLE(%theta, %phi) a b AS SEQUENCE:
  RZ(theta) a
  RX(pi / 2) a
  RZ(phi) b
  RX(pi / 2) b

SOME_CYCLE(params[0], params[1]) 0 1

When I want to run this program on the QPU, I would expand the calibrations.
When I want to simulate this program, I would expand the gate sequence definition.

Gate sequences could also be used as a convenience function for describing a program compactly:

DECLARE params REAL[3]

DEFCAL RZ(%theta) 0:
  # ...

DEFCAL RZ(pi/2) 0:
  # ...

DEFGATE ZXZXZ(%theta, %psi, %phi) a:
  RZ(%theta) a
  RZ(pi / 2) a
  RZ(%psi) a
  RZ(pi / 2) a
  RZ(%phi) a

ZXZXZ(params[0], params[1], params[2]) 0

Note that the order of desired expansion is reversed here. In the first example, we want to expand calibrations and not expand gate sequences. In this example, we must expand the gate sequence definition before calibrations. There is no presumption to make on behalf of the user here, so if they want to expand everything, they should programmatically do just that.

I can certainly add a documentation example with these examples.

antalsz

Apologies for the delay, but the resulting changes look great – I have some very minor comments, but then I think we're good to merge! 🚀

quil-py/quil/program/__init__.pyi

quil-py/src/instruction/gate.rs

quil-py/src/program/mod.rs

quil-py/tests_py/program/test_program.py

quil-rs/src/instruction/gate.rs

quil-rs/src/instruction/gate_sequence.rs

antalsz · 2025-09-30T19:32:37Z

quil-py/example.py

This file appears to be duplicated later in a Rust and a Python doc comment. Perhaps it should just live in those doc comments? On the Rust side, it's possible to include a file in a doc comment, but I don't know if it is or not in Python. A note to the developers to keep things in sync would also be nice, I think – again, this is the sort of thing that #462 will help with.

I'll make the comment and take this file out.

I didn't intend to version this file, but the reason I wrote it in the first place was because I couldn't get the doctests to actually run through pytest:

_______________________________________________________________________ ERROR collecting quil/validation/identifier.py ________________________________________________________________________ .venv/lib/python3.11/site-packages/_pytest/runner.py:344: in from_call result: TResult | None = func() ^^^^^^ .venv/lib/python3.11/site-packages/_pytest/runner.py:389: in collect return list(collector.collect()) ^^^^^^^^^^^^^^^^^^^^^^^^^ .venv/lib/python3.11/site-packages/_pytest/doctest.py:566: in collect module = self.obj ^^^^^^^^ .venv/lib/python3.11/site-packages/_pytest/python.py:280: in obj self._obj = obj = self._getobj() ^^^^^^^^^^^^^^ .venv/lib/python3.11/site-packages/_pytest/python.py:551: in _getobj return importtestmodule(self.path, self.config) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ .venv/lib/python3.11/site-packages/_pytest/python.py:498: in importtestmodule mod = import_path( .venv/lib/python3.11/site-packages/_pytest/pathlib.py:595: in import_path module_file = mod.__file__ ^^^^^^^^^^^^ E AttributeError: module 'quil.validation.identifier' has no attribute '__file__' =================================================================================== short test summary info =================================================================================== ERROR quil/validation/identifier.py - AttributeError: module 'quil.validation.identifier' has no attribute '__file__'

I believe this is a fundamental issue with stubs and extension modules, but it's been a while since I looked into it.

quil-rs/src/program/defgate_sequence_expansion.rs

asaites

Thank you for your hard work on this! And my apologies it's taken me so long to review it.

The bones here are good! I particularly like the idea of uniting the source maps, but there are some rough spots to the API that would benefit from some massaging for long-term maintainability. I need a bit more time to digest some aspects of the code, and I still intend to help rebase this on the merged crates (which should help simplify some aspects). I didn't want to make you wait longer, though, so I'm submitting this initial review.

asaites · 2025-10-07T22:50:54Z

quil-py/quil/instructions/__init__.pyi

+@final
+class GateSignature:
+    """A signature for a gate definition; this does not include the gate definition content.
+
+    To get a signature from a definition, use `GateDefinition.signature`.
+    """
+    def __new__(cls, name: str, gate_parameters: List[str], qubit_parameters: List[str], gate_type: GateType) -> Self: ...
+
+@final
+class DefGateSequence:
+    """A sequence of gates that make up a defined gate (i.e. with `DEFGATE ... AS SEQUENCE`)."""
+
+    def __new__(cls, qubits: List[str], gates: List[Gate]) -> Self:
+        """Creates a new `DefGateSequence` with the given qubits and gates.
+
+        :param qubits: A list of qubit names that the gates in the sequence will act on.
+        :param gates: A list of `Gate` objects that make up the sequence. Each gate must reference
+            qubits in the `qubits` list by name. They may not specify a fixed qubit.
+        """
+        ...
+
+    @property
+    def qubits(self) -> List[str]:
+        """Returns the list of qubit variable names in the gate signature."""
+        ...
+
+    @property
+    def gates(self) -> List[Gate]:
+        """Returns the list of `Gate` objects that make up the sequence."""
+        ...
+


When this branch gets updated to be compatible with the merged quil-py/quil-rs crates, these files will be generated automatically from the Rust source, so this documentation will need to move to their Rust counterparts (with quil-rs specifically, not wrappers for them in quil-py).

quil-py/quil/program/__init__.pyi

quil-rs/src/program/mod.rs

quil-rs/src/program/defgate_sequence_expansion.rs

quil-rs/src/program/mod.rs

asaites

I've made some suggestions based on the changes, and implemented them here. I think there's still room for improvement on the implementation details of the defgate_sequence_expansion, particularly regarding the allocation of Vecs that likely could be done by passing along a target vec to fill -- but if you're happy with the API, then we can address that if it becomes a performance problem in the future.

asaites · 2025-10-30T20:39:46Z

quil-rs/src/instruction/gate.rs

                }
            }
+            GateSpecification::Sequence(sequence) => {
+                for gate in sequence.gates.iter() {


Nit/clippy suggestion:

Suggested change

for gate in sequence.gates.iter() {

for gate in &sequence.gates {

asaites · 2025-10-31T16:44:27Z

quil-rs/src/instruction/gate_sequence.rs

+                let gate_parameters = gate
+                    .parameters
+                    .iter()
+                    .cloned()


These don't actually need to be cloned().

Suggested change

.cloned()

asaites · 2025-10-31T17:01:13Z

quil-rs/src/instruction/mod.rs

+        f,
+        parameters
+            .iter()
+            .map(|p| p.as_ref())


Nit:

Suggested change

.map(|p| p.as_ref())

.map(AsRef::as_ref)

asaites · 2025-10-31T17:08:15Z

quil-rs/src/program/defgate_sequence_expansion.rs

+        gate_expansion_stack: &mut IndexSet<String>,
+    ) -> Result<Vec<Instruction>, DefGateSequenceExpansionError> {
+        let mut target_instructions = vec![];
+        for source_instruction in source_instructions.iter() {


Suggested change

for source_instruction in source_instructions.iter() {

for source_instruction in source_instructions {

asaites · 2025-10-31T17:25:02Z

quil-rs/src/program/source_map.rs

    where
        TargetIndex: SourceMapIndexable<QueryIndex>,
    {
        self.entries


Obviously this was here before, but while we're making this change, I think this would be a lot cleaner as the (equivalent) filter followed by map:

self.entries .iter() .filter(|&entry| entry.target_location().contains(target_index)) .map(SourceMapEntry::source_location) .collect()

asaites · 2025-10-31T22:32:09Z

quil-rs/src/program/defgate_sequence_expansion.rs

+    fn gate_sequence_from_instruction(
+        &self,
+        instruction: &Instruction,
+        gate_expansion_stack: &mut IndexSet<String>,


Suggested change

gate_expansion_stack: &mut IndexSet<String>,

gate_expansion_stack: &IndexSet<String>,

asaites · 2025-11-01T02:11:44Z

quil-rs/src/program/defgate_sequence_expansion.rs

+                let mut gate_expansion_stack = gate_expansion_stack.clone();
+                gate_expansion_stack.insert(gate_sequence_signature.name().to_string());


Since you're constructing a new one here and then dropping it at the end of the block, it doesn't actually need to be a &mut reference parameter to the function. That said, it probably makes more sense to keep it that way, but avoid the clone here. In order to do that, you'd need to check the result of this insert and (if it's true), you'd pop the name back off the stack once the recursive call finishes. But since that might be easy to mess up, I'd actually recommend making a little wrapper and giving it a method that manages that for the caller:

struct ExpansionStack(IndexSet<String>); impl ExpansionStack { fn new() -> Self { Self(IndexSet::new()) } /// Check if the name is in the stack and, if so, return an error. fn check(&self, name: impl AsRef<str>) -> Result<(), DefGateSequenceExpansionError> { if self.0.contains(name.as_ref()) { let cycle = self.0.iter().cloned().collect(); Err(DefGateSequenceExpansionError::CyclicSequenceGateDefinition(cycle)) } else { Ok(()) } } /// Execute a closure with an gate added to the stack. fn with_gate_sequence<F, R>(&mut self, name: String, f: F) -> R where F: FnOnce(&mut Self) -> R, { let must_pop = self.0.insert(name); let result = f(self); if must_pop { self.0.pop(); } result } }

Then these signatures would accept &mut ExpansionStack and use something like this:

let mut recursive_source_map = SourceMap::default(); let recursive_target_gate_instructions = gate_expansion_stack.with_gate_sequence( gate_sequence_signature.name().to_string(), |gate_expansion_stack| self.expand_with_source_map_impl( &target_gate_instructions, &mut recursive_source_map, gate_expansion_stack, ) )?;

asaites · 2025-11-01T02:20:01Z

quil-rs/src/program/mod.rs

+        .iter()
+        .filter(|(name, _)| !filter(name))
+    {
+        for (gate_name, (j, _)) in gate_sequence_definitions.iter() {


Suggested change

for (gate_name, (j, _)) in gate_sequence_definitions.iter() {

for (gate_name, (j, _)) in &gate_sequence_definitions {

asaites · 2025-11-01T02:31:02Z

quil-rs/src/program/quilpy.rs

+#[derive(Debug)]
+#[cfg_attr(feature = "stubs", gen_stub_pyclass)]
+#[pyclass(module = "quil.program", frozen)]
+pub struct DefGateExpansionFilter {
+    filter: Py<PyFunction>,
+    on_error: Py<PyFunction>,
+}
+
+#[pymethods]
+impl DefGateExpansionFilter {
+    #[new]
+    #[pyo3(signature = (/, filter, on_error))]
+    fn new(filter: Py<PyFunction>, on_error: Py<PyFunction>) -> Self {
+        Self { filter, on_error }
+    }
+}


Although this works, I recommend we just accept a single Callable for filtering, and not have any an on_error of any sort. Here's my reasoning:

The filter_instructions method already works this way, and from an API perspective, it'd be nicer for these to have the same form. Changing the existing one is a more impactful than having this match it.

For a similar reason, we should probably call this predicate rather than filter; that's also a little nicer since filter is a Python builtin function.

Since this function comes from the user, if there's an error while calling it, it's almost certainly a bug in their application and something they should address.

If they do have a reason to pass a filter that might return an invalid value, they have other options for handling it.

Calling on_error could cause a panic just as well.

If our code fails for reasons unrelated to calling their function, we certainly want a noisy crash.

So my recommendation is to just accept a predicate argument and panic on mistakes.

quil-rs/src/program/quilpy.rs

* style: use same predicate semantics across API Currently `filter_instructions` uses the name "predicate", an ordinary Python `Callable`, and bubbles errors as `PanicException`s. This edits the new defgate functions to use the same naming and semantics. It also introduces a `call_predicate` function to simplify the common logic associated with calling those predicates, and refactors the relevant to code to make use of it. * style: refactor defgate seq expander with stack * style: apply clippy suggestions * chore: regenerate stubs * style: remove line * fix: self import for python 3.10 --------- Co-authored-by: Alexander Saites <[email protected]>

windsurf-bot bot reviewed Jul 13, 2025

View reviewed changes

quil-rs/src/filter_set.rs Outdated Show resolved Hide resolved

quil-rs/src/instruction/gate_sequence.rs Outdated Show resolved Hide resolved

quil-rs/src/program/mod.rs Outdated Show resolved Hide resolved

erichulburd changed the title ~~Draft: feat: defgate as sequence~~ feat: defgate as sequence Jul 22, 2025

antalsz requested changes Aug 5, 2025

View reviewed changes

antalsz changed the title ~~feat: defgate as sequence~~ feat!: defgate as sequence Aug 5, 2025

erichulburd force-pushed the 447-defgate_as_sequence branch 2 times, most recently from 6bdae28 to 8ce18b0 Compare August 13, 2025 20:29

erichulburd requested a review from antalsz August 21, 2025 20:42

antalsz approved these changes Sep 30, 2025

View reviewed changes

erichulburd force-pushed the 447-defgate_as_sequence branch from ef4b0ab to ee6f91e Compare October 2, 2025 18:55

asaites reviewed Oct 9, 2025

View reviewed changes

asaites force-pushed the 447-defgate_as_sequence branch 2 times, most recently from 07c11d5 to e3d5005 Compare October 17, 2025 02:04

feat: defgate as sequence

043e9e2

asaites force-pushed the 447-defgate_as_sequence branch from e3d5005 to 043e9e2 Compare October 17, 2025 16:22

erichulburd added 2 commits October 20, 2025 09:19

chore: accommodate new review

657e96e

chore: address more pull request feedback

7bb64a8

erichulburd mentioned this pull request Oct 20, 2025

447 defgate as sequence bak #486

Closed

erichulburd added 7 commits October 20, 2025 12:24

docs: defgate sequence expansion docs

a487efa

refactor: filter to take &str

c06f122

docs: python bindings

da50fc8

feat: support expansion filter

a59cdba

docs: fix plural typo

2949f5d

chore: format source

8e64ded

feat!: empty commit for version bump

8c3d2a3

asaites reviewed Nov 1, 2025

View reviewed changes

asaites merged commit 889793a into main Nov 5, 2025
19 checks passed

asaites deleted the 447-defgate_as_sequence branch November 5, 2025 23:59

	for gate in sequence.gates.iter() {
	for gate in &sequence.gates {

	for source_instruction in source_instructions.iter() {
	for source_instruction in source_instructions {

	gate_expansion_stack: &mut IndexSet<String>,
	gate_expansion_stack: &IndexSet<String>,

		let mut gate_expansion_stack = gate_expansion_stack.clone();
		gate_expansion_stack.insert(gate_sequence_signature.name().to_string());

	for (gate_name, (j, _)) in gate_sequence_definitions.iter() {
	for (gate_name, (j, _)) in &gate_sequence_definitions {

feat!: defgate as sequence #460

feat!: defgate as sequence #460

Uh oh!

Conversation

erichulburd commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Features

Refactor

Notes on approach

Uh oh!

github-actions bot commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

windsurf-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antalsz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichulburd commented Aug 6, 2025

Uh oh!

antalsz left a comment • 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!

Uh oh!

Uh oh!

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!

Uh oh!

asaites left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asaites left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

erichulburd commented Jul 13, 2025 •

edited

Loading

github-actions bot commented Jul 13, 2025 •

edited

Loading

antalsz left a comment •

edited

Loading