feat: add source mapping for calibration expansion #370

kalzoo · 2024-05-04T06:35:47Z

This PR allows the (Rust package) user to do the following:

Expand a single instruction in a self-descriptive, recursive way. That is, expand an instruction using its calibrations, and alongside the newly expanded instructions, receive an object describing the calibration decisions made and which of the new instructions are a result of which calibration decisions.
Expand an entire program in the same way, receiving not only the expanded program but a source mapping explaining how every instruction in the new program was derived from the first one: either it was expanded (and it will show the expanded instruction as well as the DEFCAL [MEASURE]s used to expand it) or it was not (and it will yield the instruction index in both source and target program which contains it)

Reviewer:

Please consider the two comments suggesting a breaking change to the MeasureCalibrationDefinition and Calibration structs. This would make them somewhat more similar to DEFFRAME in that they would have identifiers that could be used to index them without respect to their instruction bodies. We could also take the chance to rename Calibration to CalibrationDefinition for consistency.
- I have made this change, but in a way that should not break some python consumers.
- All of the original @propertys are there, so this will only break users who construct and modify calibrations and measure calibrations. I could add additional setters to help migrate, but there's only one __new__ and it doesn't feel right to leave those behind.
Please take a look at the new tests and consider the SourceMap format. Do you see any shortcomings?
Please look at the CalibrationSet#_expand method and those nearby.
- I'd like to have this only construct a CalibrationExpansion when that is returned to the user; I first tried an expansion: Option<&mut CalibrationExpansion> argument, but that doesn't work well here because that doesn't have a sensible default.
- So instead I went with build_source_map which skips most of the work involved in calibration expansion source mapping while still technically constructing one which is then discarded prior to return from CalibrationSet.expand.
- Can you suggest a more elegant solution? I want to share logic between expand-with-source-map and expand-without-source-map while not doing any source mapping work if one isn't going to be returned.

TODOs:

Fix the bug in program calibration expansion source mapping
Restore the "fast path" to instruction calibration expansion when no source map is desired. In the current implementation, it's built in all cases and then thrown away in some in order to maintain the existing API.
(maybe, at request) An ergonomic reverse-mapping to allow looking up the source instruction that resulted in a given target instruction. That can always be built by the user, but it might be nice to have one here since the source mapping structure takes a moment to grok even for an experienced Quil dev.
(maybe) implement source mapping for parsing. This could be done on an experimental branch just to validate that this PR has the right API to support that.

Closes #366

github-actions · 2024-05-04T06:36:51Z

PR Preview Action v1.4.8
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-370/
on branch `quil-py-docs` at 2024-10-12 00:42 UTC

mhodson-rigetti · 2024-05-06T06:12:41Z

As much as I can read Rust, this is making sense to me. On the first point about a lurking bug, I wonder if impl SourceMapRange for CalibrationExpansion contains() is incomplete as containment is only by the index being valid, not that it also came from the same calibration? On the third point about reverse-mapping help, if I could see the pattern for this implemented using only the Python bindings as a recipe I think I'd be fine, but if doing that means you might as well codify on the Rust side, happy for that also!

…Definition

…Identifier

…ly built when returned to the user

kalzoo · 2024-05-09T04:32:37Z

As much as I can read Rust, this is making sense to me. On the first point about a lurking bug, I wonder if impl SourceMapRange for CalibrationExpansion contains() is incomplete as containment is only by the index being valid, not that it also came from the same calibration? On the third point about reverse-mapping help, if I could see the pattern for this implemented using only the Python bindings as a recipe I think I'd be fine, but if doing that means you might as well codify on the Rust side, happy for that also!

Hey, thanks @mhodson-rigetti . I've added python bindings and docs but no example (yet) - I'll have to think about how best to expose those direct mappings from source to target. Right now (following our chat a week ago) I made the structure recursive to model the recursive nature of calibration expansion, and that's the least intuitive part in practical use, because each level of expansion has its own index basis. There's an example describing that problem in the python docstrings now.

You were right about SourceMapRange for exactly that reason - two ranges might not share the same basis and the SourceMapRange doesn't have enough information to make that clear. I just removed SourceMapRange altogether in this iteration, I don't think this functionality needs to be so general-purpose just yet.

The lingering bug though was actually fixed in this commit and was something prompted by our chat a week ago - certain instructions being yoinked out of the instruction body and thus (necessarily) out the source map.

kalzoo · 2024-05-10T18:15:33Z

quil-rs/src/instruction/calibration.rs

        })
    }
 }

+impl CalibrationSignature for Calibration {


From @MarquessV : this should not be needed anymore if the CalibrationIdentifier can serve as the signature

On second thought here, CalibrationSignature still seems necessary for CalibrationSet to be generic, and it seems to make sense for impl CalibrationSignature for Calibration to defer to impl CalibrationSignature for CalibrationIdentifier. I have a feeling I'm being dense about this so lmk if you see otherwise

That is true, I don't know of a good way to make the generics work with just a CalibrationIdentifier struct. I agree that it makes sense to formalize the concept of a calibration identifier w/ a struct and defer to it to implement the signature logic. They serve different purposes, so it's not entirely redundant, and the core logic doesn't need to be duplicated between either, so I'm good with this.

quil-rs/src/instruction/calibration.rs

kalzoo · 2024-05-10T18:17:44Z

quil-rs/src/program/mod.rs

+                    expansion_output
+                        .detail
+                        .remove_target_index(relative_target_index);


This feels very clumsy and likely inefficient, but no more clever way occurred to me. Suggestions welcome.

Basically, we determine whether an instruction is a "header" or "body" instruction by whether or not it made the instruction body longer, and then based on that we excise it from the source mapping so that the target_index ranges stay accurate and don't map to a missing instruction

I don't necessarily have a better idea for this, but an observation that the distinction between "header" and "body" instruction comes up enough that maybe we should have a more concrete definition of it in the library? Like a method in Instruction maybe? I know it's not a part of the spec, but it is a practical one that this library makes decisions around.

I think @MarquessV is exactly right; this seems like it's going to be much cleaner if it never gets added than if it gets added and deleted. In particular, if the calibration expansion can check if something is a "header" instruction, then it can refrain from adding it to the expanded source, and then we don't need to do this fixup.

if the calibration expansion can check if something is a "header" instruction, then it can refrain from adding it to the expanded source, and then we don't need to do this fixup.

I don't see how this is possible. When the instruction is expanded, it does not have the scope of a program, and so it is just expanded into a Vec<Instruction>. However, when we add that Vec to a program here, then some of its members are "hoisted" into data structures like memory region declarations and calibration declarations and such. Expanding the calibration in such a way that "predicted" how the program would handle it, without the use of an actual program, is something that could fall out of sync and requires extra care and maintenance.

It's statically knowable which instructions get hoisted and which ones don't: Instruction::CalibrationDefinition does, Instruction::Gate doesn't, etc. Expansion could return two vectors: Vec<HoistedInstruction> and Vec<ComputationInstruction> (new types optional, but perhaps clearer). Then anything working on the results of expansion wouldn't have to do its own classification.

quil-py/src/program/source_map.rs

quil-rs/src/program/calibration.rs

MarquessV · 2024-05-10T20:19:25Z

quil-rs/src/program/mod.rs

+                    expansion_output
+                        .detail
+                        .remove_target_index(relative_target_index);


I don't necessarily have a better idea for this, but an observation that the distinction between "header" and "body" instruction comes up enough that maybe we should have a more concrete definition of it in the library? Like a method in Instruction maybe? I know it's not a part of the spec, but it is a practical one that this library makes decisions around.

antalsz

Looks overall good to me, but I think some details need to be hashed out before we merge this

quil-rs/src/program/source_map.rs

quil-rs/src/program/mod.rs

antalsz · 2024-06-28T21:58:28Z

quil-rs/src/program/mod.rs

+                    expansion_output
+                        .detail
+                        .remove_target_index(relative_target_index);


I think @MarquessV is exactly right; this seems like it's going to be much cleaner if it never gets added than if it gets added and deleted. In particular, if the calibration expansion can check if something is a "header" instruction, then it can refrain from adding it to the expanded source, and then we don't need to do this fixup.

antalsz · 2024-06-28T21:59:21Z

quil-rs/src/program/mod.rs

@@ -693,14 +773,38 @@ impl ops::AddAssign<Program> for Program {
    }
 }

+type InstructionIndex = usize;


Are we sure we want this to be a type synonym and not a newtype? It's probably fine but I figure it's worth being explicit about that choice

I realize I had missed this comment on the last round, but it's a good idea. Changed that now.

kalzoo

Thanks for the review, @antalsz

quil-py/src/program/source_map.rs

kalzoo · 2024-07-22T16:30:35Z

quil-rs/src/instruction/calibration.rs

        })
    }
 }

+impl CalibrationSignature for Calibration {


On second thought here, CalibrationSignature still seems necessary for CalibrationSet to be generic, and it seems to make sense for impl CalibrationSignature for Calibration to defer to impl CalibrationSignature for CalibrationIdentifier. I have a feeling I'm being dense about this so lmk if you see otherwise

quil-rs/src/instruction/calibration.rs

quil-rs/src/program/calibration.rs

antalsz

These changes look good, thanks! I have a few open threads that weren't addressed, but none I'd consider blockers (except for this one new comment). However, the discussion around header/body instruction distinction should wrap up before we merge this.

quil-rs/src/instruction/calibration.rs

antalsz · 2024-07-22T21:30:34Z

quil-rs/src/program/calibration.rs

    pub(crate) fn remove_target_index(&mut self, target_index: usize) {
-        eprintln!("Removing target index {} from\n{self:#?}", target_index);
-
+        // Adjust the start of the range if the target index is within the range


Suggested change

// Adjust the start of the range if the target index is within the range

// Adjust the start of the range if the target index is before the range

quil-rs/src/instruction/calibration.rs

quil-rs/src/program/calibration.rs

antalsz · 2024-07-22T21:53:27Z

quil-rs/src/program/mod.rs

+                    expansion_output
+                        .detail
+                        .remove_target_index(relative_target_index);


It's statically knowable which instructions get hoisted and which ones don't: Instruction::CalibrationDefinition does, Instruction::Gate doesn't, etc. Expansion could return two vectors: Vec<HoistedInstruction> and Vec<ComputationInstruction> (new types optional, but perhaps clearer). Then anything working on the results of expansion wouldn't have to do its own classification.

antalsz

As I said in my last review – now that I understand the negation change, I would not consider any of my remaining comments blocking. Approved!

antalsz

Reviewed 5d31908; a couple minor thoughts about the explanation, but the overall concept is great.

antalsz · 2024-07-29T22:16:08Z

quil-py/quil/program/__init__.pyi

+    # This is what we expect the expanded program to be. X and Y have been replaced by Z
+    expected_program_text = inspect.cleandoc(
+        \"\"\"
+        DEFCAL X 0:
+            Y 0
+
+        DEFCAL Y 0:
+            Z 0
+
+        Z 0 # This instruction is index 0
+        Z 0 # This instruction is index 1
+        \"\"\"
+    )


I think this ought to have an assert. That raises questions of the precise whitespace, I know. Could we parse them both to Quil and assert the parsed programs are equal?

the test did, I left it out here for readability but have restored it now

antalsz · 2024-07-29T22:19:58Z

quil-py/quil/program/__init__.pyi

+    # In order to discover _which_ calibration led to each Z in the resulting program, we
+    # can interrogate the expansion source mapping:


I like this section a lot, but I found the specific comments a bit confusing. Perhaps we could revise this first comment to something like

"…can interrogate the expansion source mapping. For instance, The X at index 0 should have been replaced with a Z at index 0. Here's how we could confirm that: First, we…".

As it stands, the comment on line 46 made me think that we were also going to check the Y at index 1. (I think adding that would probably be redundant.)

Another possibility would be to drop the ellipses on lines 51 and 63; they're not following on from a previous sentence.

quil-py/quil/program/__init__.pyi

antalsz · 2024-07-29T22:21:26Z

quil-py/quil/program/__init__.pyi

+
+## Source Mapping for Calibration Expansion
+
+```py


Is this snippet run and tested? It would be nice if it were but I don't know enough about Python to tell if that's happening/how easy it would be to have that happen.

yes, it's a test case, although not tested in situ unfortunately

Can we add a pointer to the test case it corresponds to?

bramathon · 2024-10-04T08:42:05Z

Just wanted to check on the status of this - it's important for pulse plotting to work.

bramathon · 2024-10-05T20:43:47Z

I'm a little surprised by the extent of the changes in this PR. I was expecting something more along the lines of this:

from quil.program import Program
from typing import Tuple, List

def expand_with_source_mapping(program: Program) -> Tuple[Program, List[int]]:
    """
    Expand any instructions in the program which have a matching calibration, leaving the others
    unchanged. 

    :param program: A quil.Program.
    :return: A quil.Program with the instructions expanded and a source map.
    The source map is a list with the length of the expanded instructions, and indicates the index
    of the logical instruction in the original program from which the expanded instruction originated.
    """
    instructions = program.body_instructions
    calibrations = program.calibrations
    expanded_program = program.clone_without_body_instructions()

    source_map = []
    expanded_instructions = []
    for logical_index, inst in enumerate(instructions):
        expanded_instruction = calibrations.expand(inst, [])
        source_map += [logical_index]*len(expanded_instruction)
        expanded_instructions += expanded_instruction
    expanded_program.add_instructions(expanded_instructions)
    return expanded_program, source_map

expanded_program, source_map = expand_with_source_mapping(quil_program)
default_expanded_program = quil_program.expand_calibrations()
assert expanded_program == default_expanded_program

kalzoo · 2024-10-11T19:00:17Z

I'm a little surprised by the extent of the changes in this PR. I was expecting something more along the lines of this:

from quil.program import Program
from typing import Tuple, List

def expand_with_source_mapping(program: Program) -> Tuple[Program, List[int]]:
    """
    Expand any instructions in the program which have a matching calibration, leaving the others
    unchanged. 

    :param program: A quil.Program.
    :return: A quil.Program with the instructions expanded and a source map.
    The source map is a list with the length of the expanded instructions, and indicates the index
    of the logical instruction in the original program from which the expanded instruction originated.
    """
    instructions = program.body_instructions
    calibrations = program.calibrations
    expanded_program = program.clone_without_body_instructions()

    source_map = []
    expanded_instructions = []
    for logical_index, inst in enumerate(instructions):
        expanded_instruction = calibrations.expand(inst, [])
        source_map += [logical_index]*len(expanded_instruction)
        expanded_instructions += expanded_instruction
    expanded_program.add_instructions(expanded_instructions)
    return expanded_program, source_map

expanded_program, source_map = expand_with_source_mapping(quil_program)
default_expanded_program = quil_program.expand_calibrations()
assert expanded_program == default_expanded_program

@bramathon that snippet doesn't:

break out recursive calibrations
capture which calibration was used to do the expansion

Some of the diff, also, improves how calibrations are handled and matched

bramathon · 2024-10-11T19:09:29Z

Yup, that's true. I don't have any need for those things for my use case.

…

On Fri, Oct 11, 2024, 20:00 Kalan ***@***.***> wrote: I'm a little surprised by the extent of the changes in this PR. I was expecting something more along the lines of this: from quil.program import Programfrom typing import Tuple, List def expand_with_source_mapping(program: Program) -> Tuple[Program, List[int]]: """ Expand any instructions in the program which have a matching calibration, leaving the others unchanged. :param program: A quil.Program. :return: A quil.Program with the instructions expanded and a source map. The source map is a list with the length of the expanded instructions, and indicates the index of the logical instruction in the original program from which the expanded instruction originated. """ instructions = program.body_instructions calibrations = program.calibrations expanded_program = program.clone_without_body_instructions() source_map = [] expanded_instructions = [] for logical_index, inst in enumerate(instructions): expanded_instruction = calibrations.expand(inst, []) source_map += [logical_index]*len(expanded_instruction) expanded_instructions += expanded_instruction expanded_program.add_instructions(expanded_instructions) return expanded_program, source_map expanded_program, source_map = expand_with_source_mapping(quil_program)default_expanded_program = quil_program.expand_calibrations()assert expanded_program == default_expanded_program @bramathon <https://github.com/bramathon> that snippet doesn't: - break out recursive calibrations - capture which calibration was used to do the expansion Some of the diff, also, improves how calibrations are handled and matched — Reply to this email directly, view it on GitHub <#370 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEWA7XSPPA5YGRQB4JXV4DZ3AN5PAVCNFSM6AAAAABHGSS7PSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXHE3TEMZVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kalzoo · 2024-10-12T01:19:07Z

Over to you now @bramathon

feat: add source mapping for calibration expansion

1a1e8a6

kalzoo added 15 commits May 7, 2024 13:15

chore: improve docs

3a319f1

fix bug in target range mapping in calibration expansion

e13abf7

WIP: header-aware program calibration expansion

917ba23

fix bug in program calibration expansion into header instructions

bf246b3

remove unnecessary SourceMapRange trait

277105d

add source map python bindings

b35f8fe

feat!: refactor identifiers out of Calibration and MeasureCalibration…

1bec330

…Definition

add and fix py stubs for CalibrationIdentifier and MeasureCalibration…

ec90415

…Identifier

reorder bindings file in lex order

2ecf658

add source-map py stubs

8446b61

Add py stub for Program.expand_calibrations_with_source_map

feaa61c

chore: docs

00119a6

chore: fmt

1965440

refactor instruction calibration expansion so that source maps are on…

8906a76

…ly built when returned to the user

update snapshot

645e1a2

chore: lint

cafe117

kalzoo commented May 10, 2024

View reviewed changes

MarquessV reviewed May 10, 2024

View reviewed changes

antalsz requested changes Jun 28, 2024

View reviewed changes

kalzoo added 7 commits July 1, 2024 00:55

chore: cleanup

3216ef3

add query (lookup) operations for SourceMap

ac63749

fix bug in calibration expansion

4b61fc8

chore: lint

07ae1d3

fix range return type

78df471

add test

25bd576

chore: cleanup

f64aa8a

kalzoo added 4 commits July 22, 2024 08:54

chore: fix docs

d3abce6

chore: fix test

c97dcca

chore: cleanup

8ffb15a

chore: cleanup

2ca8eea

kalzoo commented Jul 22, 2024

View reviewed changes

antalsz reviewed Jul 22, 2024

View reviewed changes

antalsz approved these changes Jul 26, 2024

View reviewed changes

kalzoo added 6 commits July 29, 2024 10:57

chore: fix comment

8f91186

fix test

56476ca

make test more readable

5d9f759

chore: fix readme

59daee1

chore: lint

7a5972c

add useful example to Python docs

5d31908

antalsz reviewed Jul 29, 2024

View reviewed changes

kalzoo added 2 commits July 29, 2024 15:27

fix doc example

de94d05

fix doc example

5c05993

kalzoo force-pushed the 366-source-mapping branch from 58217f3 to 5c05993 Compare July 29, 2024 22:32

kalzoo added 2 commits July 29, 2024 15:40

Merge branch 'main' into 366-source-mapping

a879779

chore: lint

a298437

Merge branch 'main' into 366-source-mapping

9a3b453

kalzoo added 2 commits October 11, 2024 17:34

make InstructionIndex a newtype struct for source mapping

022900a

add missing from_ methods to new python bindings

f72ae40

kalzoo merged commit 9256b95 into main Oct 12, 2024
15 checks passed

kalzoo deleted the 366-source-mapping branch October 12, 2024 01:18

	// Adjust the start of the range if the target index is within the range
	// Adjust the start of the range if the target index is before the range

		# In order to discover _which_ calibration led to each Z in the resulting program, we
		# can interrogate the expansion source mapping:

feat: add source mapping for calibration expansion #370

feat: add source mapping for calibration expansion #370

Conversation

kalzoo commented May 4, 2024 • edited Loading

github-actions bot commented May 4, 2024 • edited Loading

mhodson-rigetti commented May 6, 2024

kalzoo commented May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalsz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalsz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalsz left a comment

Choose a reason for hiding this comment

antalsz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalzoo Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramathon commented Oct 4, 2024

bramathon commented Oct 5, 2024

kalzoo commented Oct 11, 2024

bramathon commented Oct 11, 2024 via email

kalzoo commented Oct 12, 2024

kalzoo commented May 4, 2024 •

edited

Loading

github-actions bot commented May 4, 2024 •

edited

Loading

kalzoo commented May 9, 2024 •

edited

Loading

antalsz left a comment •

edited

Loading

kalzoo Jul 29, 2024 •

edited

Loading