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

feat: add source mapping for calibration expansion #370

Merged
merged 41 commits into from
Oct 12, 2024
Merged
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
1a1e8a6
feat: add source mapping for calibration expansion
kalzoo May 4, 2024
3a319f1
chore: improve docs
kalzoo May 7, 2024
e13abf7
fix bug in target range mapping in calibration expansion
kalzoo May 7, 2024
917ba23
WIP: header-aware program calibration expansion
kalzoo May 7, 2024
bf246b3
fix bug in program calibration expansion into header instructions
kalzoo May 9, 2024
277105d
remove unnecessary SourceMapRange trait
kalzoo May 9, 2024
b35f8fe
add source map python bindings
kalzoo May 9, 2024
1bec330
feat!: refactor identifiers out of Calibration and MeasureCalibration…
kalzoo May 9, 2024
ec90415
add and fix py stubs for CalibrationIdentifier and MeasureCalibration…
kalzoo May 9, 2024
2ecf658
reorder bindings file in lex order
kalzoo May 9, 2024
8446b61
add source-map py stubs
kalzoo May 9, 2024
feaa61c
Add py stub for Program.expand_calibrations_with_source_map
kalzoo May 9, 2024
00119a6
chore: docs
kalzoo May 9, 2024
1965440
chore: fmt
kalzoo May 9, 2024
8906a76
refactor instruction calibration expansion so that source maps are on…
kalzoo May 9, 2024
645e1a2
update snapshot
kalzoo May 9, 2024
cafe117
chore: lint
kalzoo May 10, 2024
3216ef3
chore: cleanup
kalzoo Jul 1, 2024
ac63749
add query (lookup) operations for SourceMap
kalzoo Jul 22, 2024
4b61fc8
fix bug in calibration expansion
kalzoo Jul 22, 2024
07ae1d3
chore: lint
kalzoo Jul 22, 2024
78df471
fix range return type
kalzoo Jul 22, 2024
25bd576
add test
kalzoo Jul 22, 2024
f64aa8a
chore: cleanup
kalzoo Jul 22, 2024
d3abce6
chore: fix docs
kalzoo Jul 22, 2024
c97dcca
chore: fix test
kalzoo Jul 22, 2024
8ffb15a
chore: cleanup
kalzoo Jul 22, 2024
2ca8eea
chore: cleanup
kalzoo Jul 22, 2024
8f91186
chore: fix comment
kalzoo Jul 29, 2024
56476ca
fix test
kalzoo Jul 29, 2024
5d9f759
make test more readable
kalzoo Jul 29, 2024
59daee1
chore: fix readme
kalzoo Jul 29, 2024
7a5972c
chore: lint
kalzoo Jul 29, 2024
5d31908
add useful example to Python docs
kalzoo Jul 29, 2024
de94d05
fix doc example
kalzoo Jul 29, 2024
5c05993
fix doc example
kalzoo Jul 29, 2024
a879779
Merge branch 'main' into 366-source-mapping
kalzoo Jul 29, 2024
a298437
chore: lint
kalzoo Jul 29, 2024
9a3b453
Merge branch 'main' into 366-source-mapping
kalzoo Oct 11, 2024
022900a
make InstructionIndex a newtype struct for source mapping
kalzoo Oct 12, 2024
f72ae40
add missing from_ methods to new python bindings
kalzoo Oct 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions quil-py/quil/program/__init__.pyi
Original file line number Diff line number Diff line change
@@ -1,3 +1,70 @@
"""
The `quil.program` module contains classes for constructing and representing a Quil program.

# Examples

## Source Mapping for Calibration Expansion

```py
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kalzoo kalzoo Jul 29, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

import inspect
antalsz marked this conversation as resolved.
Show resolved Hide resolved

program_text = inspect.cleandoc(
\"\"\"
DEFCAL X 0:
Y 0

DEFCAL Y 0:
Z 0

X 0 # This instruction is index 0
Y 0 # This instruction is index 1
\"\"\"
)

# First, we parse the program and expand its calibrations
program = Program.parse(program_text)
expansion = program.expand_calibrations_with_source_map()
source_map = expansion.source_map()

# 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
\"\"\"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, fixed


# The X at index 0 should have been replaced with a Z at index 0:

# ...first, we list the calibration expansion targets for that first instruction...
targets = source_map.list_targets_for_source_index(0)

# ...then we extract the expanded instruction.
# If the instruction had _not_ been expanded (i.e. there was no matching calibration), then `as_expanded()` would return `None`.
expanded = targets[0].as_expanded()

# ...This line shows how that `X 0` was expanded into instruction index 0 (only) within the expanded program.
# The end of the range is exclusive.
assert expanded.range() == range(0, 1)

# We can also map instructions in reverse: given an instruction index in the expanded program, we can find the source index.
# This is useful for understanding the provenance of instructions in the expanded program.
sources = source_map.list_sources_for_target_index(1)

# ... in this case, the instruction was expanded from the source program at index 1.
assert sources == [1]
```
"""

from typing import Callable, Dict, FrozenSet, List, Optional, Sequence, Set, Union, final

import numpy as np
Expand Down