feat!: Change the return type of measurement functions to a new Measurement type and remove opaque bools from the compiler#1707
feat!: Change the return type of measurement functions to a new Measurement type and remove opaque bools from the compiler#1707tatiana-s wants to merge 21 commits into
Measurement type and remove opaque bools from the compiler#1707Conversation
# Conflicts: # guppylang/src/guppylang/std/qsystem/__init__.py # guppylang/src/guppylang/std/qsystem/functional.py
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
|
| Branch | ts/future-measure |
| Testbed | Linux |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold | 158.29 x 1e3(-0.30%)Baseline: 158.77 x 1e3 | 160.36 x 1e3 (98.71%) | 📈 view plot 🚷 view threshold | 6,630.00(-0.17%)Baseline: 6,641.00 | 6,707.41 (98.85%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 26.98 x 1e3(-2.00%)Baseline: 27.53 x 1e3 | 27.81 x 1e3 (97.03%) | 📈 view plot 🚷 view threshold | 1,051.00(-2.14%)Baseline: 1,074.00 | 1,084.74 (96.89%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_benchmark_compile | 📈 view plot 🚷 view threshold | 10.08 x 1e3(-7.61%)Baseline: 10.91 x 1e3 | 11.02 x 1e3 (91.48%) | 📈 view plot 🚷 view threshold | 300.00(-2.60%)Baseline: 308.00 | 311.08 (96.44%) |
| tests/benchmarks/test_queue_push_pop.py::test_queue_push_pop_benchmark_compile | 📈 view plot 🚷 view threshold | 13.88 x 1e3(-6.44%)Baseline: 14.84 x 1e3 | 14.99 x 1e3 (92.64%) | 📈 view plot 🚷 view threshold | 423.00(-2.76%)Baseline: 435.00 | 439.35 (96.28%) |
# Conflicts: # pyproject.toml # uv.lock
# Conflicts: # pyproject.toml # uv.lock
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1707 +/- ##
==========================================
- Coverage 93.49% 92.89% -0.61%
==========================================
Files 133 134 +1
Lines 12793 12935 +142
==========================================
+ Hits 11961 12016 +55
- Misses 832 919 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maximilianruesch
left a comment
There was a problem hiding this comment.
Big PR, lots of little changes. Great work! I massively trust the tests on this one.
A few comments here and there. Also I think the title (which will end up as the commit title) should mention that we drop the opaque guppy bools / that we change the return type of measurement functions, not just add some type.
| # TODO this should be a compile-time error | ||
| # Fails with "pyo3_runtime.PanicException: Cannot convert negative integer -2 to | ||
| # unsigned." | ||
| @pytest.mark.xfail(reason="Rust PanicException cannot be caught from Python") |
There was a problem hiding this comment.
You can catch "Exception", and PanicException should be a subtype of that
There was a problem hiding this comment.
I did try that and it didn't work?
There was a problem hiding this comment.
What do you mean by "it did not work"? The panic went through? You can also catch BaseException even more generally...
| "metadata": {}, | ||
| "source": [ | ||
| "Simply replacing the method results in an error because `lazy_measure` returns a value of type `Measurement`. In order to obtain a `bool`, we have to explicitaly use `read()` on the value. " | ||
| "Simply replacing the method results in an error because `result` expects a `bool`. In order to obtain it, we have to explicitly use `read()` on the value of type `Measurement`. This will block until the physical measurement is completed:" |
There was a problem hiding this comment.
We are not "replacing" the method anymore, since we are breakingly forcing an upgrade. This needs a reword, potentially to just "This results in an error because ...".
Measurement type as the return type for measurement functionsMeasurement type and remove opaque bools from the compiler
maximilianruesch
left a comment
There was a problem hiding this comment.
LGTM for now. When merging / handling the latest tket release, please rerequest a review, and then I can approve.
| " qs = array(some(qubit()) for _ in range(3))\n", | ||
| " mask = array(True, False, True)\n", | ||
| " measure_mask(qs, mask)\n", | ||
| " # We need to consume the array of measurement options as they are linear.\n", |
There was a problem hiding this comment.
This comment and the next one have the same underlying reason (linear -> cannot be leaked), but both only display the truth partially.
|
|
||
| @guppy | ||
| @no_type_check | ||
| def collect_measurements( |
There was a problem hiding this comment.
I guess it's been called collect_measurements for awhile but it feels like it ought to have read in the name at least, given the measurements are already in a collection (so no collecting is done) and there's nothing in the name to say that it's reading!!
acl-cqc
left a comment
There was a problem hiding this comment.
Thanks @tatiana-s! A big PR already I admit but is this the time to add overrides to result that accept Measurement directly? (Or a followup?)
Closes #1485
Requires a breaking tket release with the extension changes in #1558 before this can be merged
BREAKING CHANGE: (guppylang.std.quantum):
measure(andmeasure_arrayby extension) now returnMeasurementinstead ofboolBREAKING CHANGE: (guppylang.std.qsystem/qsystem.functional):
measureandmeasure_reset(and by extension the array versions) now returnMeasurementinstead ofbool(this matches the behaviour oflazy_measure/lazy_measure_and_reset)BREAKING CHANGE: (guppylang_internals):
boolis now lowered toht.Boolagain instead oftket.boolThe
deferred_measurement.ipynbnotebook has been adjusted to explain the motivation for the lazy behaviour instead of contrastingmeasureandlazy_measure