-
Notifications
You must be signed in to change notification settings - Fork 2
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: conventional results post processing #593
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #593 +/- ##
==========================================
+ Coverage 91.58% 91.65% +0.07%
==========================================
Files 59 60 +1
Lines 6311 6389 +78
==========================================
+ Hits 5780 5856 +76
- Misses 531 533 +2 ☔ View full report in Codecov by Sentry. |
("as3ABdfAB[2]", ("as3ABdfAB", 2)), | ||
], | ||
) | ||
def test_reg_index_pattern_match(identifier, match: tuple[str, int] | None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't immediately obvious to me what the purpose of this test was, best to add a docstring.
_s34fd_fd[12]
should perhaps be accepted? Do we explicitly forbid register names starting with an underscore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this pattern matches the QASM2 allowed names
guppylang/hresult.py
Outdated
For results of the form ``` result("<register>", value) ``` `value` can be `{0, 1, True, | ||
False}`, wherein the register is assumed to be length 1, or lists over those values, | ||
wherein the list is taken to be the value of the entire register. | ||
|
||
For results of the form ``` result("<register>[n]", value) ``` `value` can only be `{0, | ||
1, True, False}`. The register is assumed to be at least `n+1` in size and unset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a great benefit in supporting both boolean literals and bits 0 and 1? From the hardware perspective, we probably only want bits.
from pytket.backends.backendresult import BackendResult | ||
|
||
#: Primitive data types that can be returned by a result | ||
DataPrimitive = int | float | bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by the spec description above:
result("<register>", value)
value
can be{0, 1}
and the code here.
Are floats supported or not? I'd imagine, if guppy says result("tag_for_pi", pi)
, we can return an approx value of pi as part of the results.
Or is this PR limited to bitstrings? If so, why include float
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float is allowed as a result type, it is not supported under the "register bitstring convention" adopted as part of this PR. The HResult
and Shots
classes faithfully capture all types, it is only the post-processing/conversions that are limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
def to_register_bits(self) -> dict[str, str]: | ||
"""Convert results to a dictionary of register bit values.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say I do
results = Results()
results.append("qs[0]", 1)
results.append("qs", 0)
results.append("qs[1]", 1)
results.to_register_bits()
The expected output should be {"qs": "01"}
, right? I think this works since dicts are iterated in insertion order and HResult.as_dict
keeps the order of entries
. You could consider using OrderedDict
to make this invariant explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it relies on insertion order preservation guaranteed from python 3.7 onwards.
OrderedDict
is more useful for things that require reordering these days so I will
just add a comment.
guppylang/hresult.py
Outdated
class HShots: | ||
"""Results accumulated over multiple shots.""" | ||
|
||
results: list[HResult] = field(default_factory=list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the default_factory
since you have a custom __init__
) from e | ||
counts = self.register_bitstrings(strict_lengths=True, strict_names=True) | ||
reg_sizes: dict[str, int] = { | ||
reg: len(next(iter(counts[reg]), "")) for reg in counts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the ""
default in next
needed? Shouldn't all registers be populated with at least one value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is just there to avoid having to handle unexpected stopiteration errors induced by changes to other code. It is safe to assume unpopulated registers are size 0
🤖 I have created a release *beep* *boop* --- ## [0.13.0](v0.12.2...v0.13.0) (2024-11-12) ### ⚠ BREAKING CHANGES * `prelude` module renamed to `std` ### Features * add `qubit` discard/measure methods ([#580](#580)) ([242fa44](242fa44)) * Add `SizedIter` wrapper type ([#611](#611)) ([2e9da6b](2e9da6b)) * conventional results post processing ([#593](#593)) ([db96224](db96224)) * Improve compiler diagnostics ([#547](#547)) ([90d465d](90d465d)), closes [#551](#551) [#553](#553) [#586](#586) [#588](#588) [#587](#587) [#590](#590) [#600](#600) [#601](#601) [#606](#606) * restrict result tag sizes to 256 bytes ([#596](#596)) ([4e8e00f](4e8e00f)), closes [#595](#595) ### Bug Fixes * Mock guppy decorator during sphinx builds ([#622](#622)) ([1cccc04](1cccc04)) ### Documentation * Add DEVELOPMENT.md ([#584](#584)) ([1d29d39](1d29d39)) * Fix docs build ([#639](#639)) ([bd6011c](bd6011c)) ### Miscellaneous Chores * Manually set last release commit ([#643](#643)) ([b2d569b](b2d569b)) ### Code Refactoring * rename prelude to std ([#642](#642)) ([1a68e8e](1a68e8e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Agustín Borgna <[email protected]>
Post processing of result streams, for converting to traditional distributions over bitstrings if some tagging convention is used, including conversion to a pytket BackendResult.
Convention
Tags are assumed to be a name of a bit register unless they fit the regex pattern
^([a-z][\w_]*)\[(\d+)\]$
(likereg[12]
) in which case they are assumed to refer to the nth element of a bit register.For results of the form
value
can be{0, 1}
, wherein the register is assumed to be length 1, or lists over those values, wherein the list is taken to be the value of the entire register.For results of the form
value
can only be{0, 1}
.The register is assumed to be at least
n+1
in size and unset elements are assumed to be0
.Subsequent writes to the same register/element in the same shot will overwrite.
To convert to a
BackendResult
all registers must be present in all shots, and register sizes cannot change between shots.TODO
Clear documentation of the above conventions and examples.