Skip to content

Conversation

@narmlu
Copy link
Contributor

@narmlu narmlu commented Jun 25, 2025

@narmlu narmlu requested a review from a team as a code owner June 25, 2025 19:38
@narmlu narmlu requested a review from ss2165 June 25, 2025 19:38
@narmlu narmlu requested a review from akhil-quantum June 25, 2025 19:58
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2025

Codecov Report

Attention: Patch coverage is 70.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 92.31%. Comparing base (eee77ae) to head (8ff7e20).

Files with missing lines Patch % Lines
guppylang/std/qsystem/__init__.py 70.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1057      +/-   ##
==========================================
- Coverage   92.37%   92.31%   -0.07%     
==========================================
  Files         112      112              
  Lines       10818    10848      +30     
==========================================
+ Hits         9993    10014      +21     
- Misses        825      834       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@narmlu
Copy link
Contributor Author

narmlu commented Jul 3, 2025

Everything seems to be working. The MaybeLeaked wrapper struct around the _measurement: Future[int] value has three funcs:

  1. is_leaked() calls future's .copy().read() (i.e., does not consume it)
  2. to_result() calls .read() (consumes it)
  3. discard() to consume the future, which needs to be called if to_result() was never called because it was leaked.

I'm not sure that we can omit discard, because I don't think that is_leaked should consume _measurement. Concrete example is in test_qsystem.

@narmlu narmlu requested a review from ss2165 July 3, 2025 18:48
Copy link
Contributor Author

@narmlu narmlu Jul 3, 2025

Choose a reason for hiding this comment

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

These are Marks changes in #1075. I'm happy to rebase to that branch if you want, but I'm inclined to just wait until that's merged.

@narmlu narmlu removed the request for review from akhil-quantum July 3, 2025 21:38
Comment on lines 75 to 77
q = qubit()
m = measure(q)
return m
Copy link
Member

Choose a reason for hiding this comment

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

What's the intention of this code pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't represent a "real" pattern. It is intended to make sure that things type check and that the variable q was properly consumed when measure_leaked(q) was called.

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to document that. Or remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@ss2165 ss2165 requested a review from mark-koch July 4, 2025 08:39
@ss2165 ss2165 removed their request for review July 4, 2025 08:39
Copy link
Collaborator

@mark-koch mark-koch left a comment

Choose a reason for hiding this comment

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

Looks good, but we should wait to merge until #1075 is merged

I'm not sure that we can omit discard, because I don't think that is_leaked should consume _measurement. Concrete example is in test_qsystem.

I think discard is nice to have 👍


@guppy
@no_type_check
def measure_leaked(q: qubit @ owned) -> "MaybeLeaked":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a dosctring here?

@guppy
@no_type_check
def to_result(self: "MaybeLeaked @ owned") -> Option[bool]:
"""Get the measurement result if not leaked."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something like this would be more accurate?

Suggested change
"""Get the measurement result if not leaked."""
"""Returns the measurement result or `nothing` if leaked."""

@mark-koch mark-koch added the wait to merge This PR to be merged after all dependencies are ready label Jul 4, 2025
@narmlu
Copy link
Contributor Author

narmlu commented Jul 7, 2025

@mark-koch - it looks like #1075 was merged. Anything else we should wait on for this one?

pyproject.toml Outdated
# Uncomment these to test the latest dependency version during development
# hugr = { git = "https://github.com/CQCL/hugr", subdirectory = "hugr-py", tag = "hugr-v0.20.2" }
# tket2-exts = { git = "https://github.com/CQCL/tket2", subdirectory = "tket2-exts", rev = "652a7d0" }
tket2-exts = { git = "https://github.com/CQCL/tket2", subdirectory = "tket2-exts", rev = "38d1c6f" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this rev dependency still needed? If yes we should wait for a tket-exts release before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch, we can wait until then (corresponding tket2 release PR CQCL/tket2#952)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the one you need: CQCL/tket2#951

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, exts. Got it, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

now released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped exts to the new release. Will merge when you remove wait to merge in case there are any other gotchas :).

Copy link
Member

Choose a reason for hiding this comment

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

the only concern is selene support, @jake-arkinstall thoughts on merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From @jake-arkinstall in teams:

I have no concerns, merge away. It won’t work with Selene and it’ll link error if used, but it won’t break existing usage (if it isn’t used it isn’t emitted). Selene can play catch up next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ss2165 - I think this is ready to merge. I'll do so tomorrow morning (Colorado time) unless you tell me otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

:shipit:

@narmlu narmlu added this pull request to the merge queue Jul 15, 2025
Merged via the queue into main with commit c555727 Jul 15, 2025
4 checks passed
@narmlu narmlu deleted the feat/measure-leaked branch July 15, 2025 11:41
github-merge-queue bot pushed a commit that referenced this pull request Aug 4, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.21.0](guppylang-v0.20.0...guppylang-v0.21.0)
(2025-08-04)


### ⚠ BREAKING CHANGES

* All compiler-internal and non-userfacing functionality is moved into a
new `guppylang_internals` package
* `guppy.compile(foo)` and `guppy.check(foo)` replaced with
`foo.check()` and `foo.compile()`
* default HUGR output uses compressed binary encoding.
* `guppylang.tracing.object.GuppyDefinition` moved to
`guppylang.defs.GuppyDefinition`
`guppylang.tracing.object.TypeVarGuppyDefinition` moved and renamed to
`guppylang.defs.GuppyTypeVarDefinition`
* All `to_hugr` methods on types, arguments, and parameters now require
a `ToHugrContext` `CompileableDef.compile_outer` now requires a
`ToHugrContext` `guppy.hugr_op` now passes the compiler context to the
function generating the op `CheckedFunctionDef` now implements
`MonomorphizableDef` instead of `CompileableDef`
`CompilerContext.build_compiled_def` now requires an instantiation for
the definition's type parameters The `ToHugrContext` protocol now
requires two additional methods: `type_var_to_hugr` and
`const_var_to_hugr` `CompilerContext.{compiled, worklist}` and
`CompilationEngine.compiled` are now indexed by a tuple of `DefId` and
optional `PartiallyMonomorphizedArgs`
* comptime code that previously used constant integers outside i64 will
now fail to compile.
* Capturing closures are now disabled by default. Enabling them requires
calling `guppylang.enable_experimental_features()`, however note that
they are not supported throughout the stack.

### Features

* Add `Future` type
([#1075](#1075))
([5ad7673](5ad7673))
* add error when constant integer out of bounds
([#1084](#1084))
([eee77ae](eee77ae))
* Add guppy version metadata to hugr entrypoint
([#1039](#1039))
([0eafbd9](0eafbd9)),
closes [#1037](#1037)
* Add manual registration of extensions
([#1045](#1045))
([4b42936](4b42936))
* add qsystem op for measure leaked
([#1057](#1057))
([c555727](c555727))
* add selene via optional feature and use for testing
([#1081](#1081))
([cefc70e](cefc70e))
* Add support for V and Vdg.
([#1094](#1094))
([6b0d44a](6b0d44a))
* Allow indexing on tuples
([#1038](#1038))
([0e9097e](0e9097e)),
closes [#711](#711)
* Declare WASM modules in guppy
([#942](#942))
([e1240fb](e1240fb))
* Extend comptime arguments to arbitrary non-linear types
([#1110](#1110))
([384dd8c](384dd8c))
* Make decorator return types more precise
([#1115](#1115))
([c84e8b1](c84e8b1))
* set hugr entrypoint to compiled function
([#1063](#1063))
([16bd267](16bd267))
* store used extensions and versions in HUGR metadata
([#1049](#1049))
([a9a300c](a9a300c)),
closes [#1048](#1048)
* Support arbitrary const generics via monomorphisation
([#1033](#1033))
([bcf9865](bcf9865))
* Support Python 3.12 generic syntax
([#1051](#1051))
([ab2e118](ab2e118)),
closes [#823](#823)
* Top level compile + emulate Interface
([#1127](#1127))
([5e2f595](5e2f595))
* update to hugr-py v0.13
([#1083](#1083))
([8f071c8](8f071c8))
* use `core.` prefix for metadata keys
([#1055](#1055))
([2bf0d68](2bf0d68))


### Bug Fixes

* Allow array comprehension syntax in comptime functions
([#1068](#1068))
([da8f04a](da8f04a)),
closes [#1067](#1067)
* Allow struct redefinitions for Python < 3.13
([#1108](#1108))
([959a4e4](959a4e4)),
closes [#1107](#1107)
* Correctly detect
`[@Custom](https://github.com/custom)_guppy_decorator` in nested scopes
([#1086](#1086))
([678583c](678583c))
* Fix diagnostics rendering for comptime entrypoints
([#1099](#1099))
([fdd2676](fdd2676)),
closes [#1097](#1097)
* Fix frame lookup for Python 3.12 annotation scopes
([#1120](#1120))
([a69e489](a69e489)),
closes [#1116](#1116)
* Fix hugr conversion and bounds checks on numeric literals
([#1100](#1100))
([73d5e92](73d5e92))
* Fix Jupyter notebook diagnostic rendering
([#1109](#1109))
([6002474](6002474))
* Fix nested function definitions in Python 3.12
([#1064](#1064))
([090f920](090f920))
* Stop showing temporary variables in comptime diagnostics
([#1112](#1112))
([63854c5](63854c5)),
closes [#1111](#1111)
* support comptime entrypoint
([#1079](#1079))
([721e3dd](721e3dd))
* Turn capturing closures into experimental feature
([#1065](#1065))
([a959b18](a959b18))


### Documentation

* add docstrings for emulator module
([#1131](#1131))
([b33e065](b33e065))
* add quantum and qsystem gate definitions
([#912](#912))
([32a4bbc](32a4bbc))
* Fix docstrings
([#1128](#1128))
([0aded85](0aded85))
* Improve RNG docs
([#1043](#1043))
([8640f06](8640f06))
* replace `compile_module` usage in README
([#1041](#1041))
([03ccf3a](03ccf3a))
* Update guppy examples
([#1121](#1121))
([b994655](b994655))


### Code Refactoring

* Split up into `guppylang_internals` package
([#1126](#1126))
([81d50c0](81d50c0))

---
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: Seyon Sivarajah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wait to merge This PR to be merged after all dependencies are ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for measure leaked

6 participants