-
Notifications
You must be signed in to change notification settings - Fork 17
feat: compiler for modifiers #1287
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
Conversation
first modifier compiler, with extensions imported modifier_compiler now directly compiles cfgs minor reference to tket repository with new extensions integration test for modifier_compiler minor
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
|
| Branch | feat/modifier-compiler |
| 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 | 142.66 x 1e3(+0.10%)Baseline: 142.52 x 1e3 | 143.94 x 1e3 (99.11%) | 📈 view plot 🚷 view threshold | 6,590.00(0.00%)Baseline: 6,590.00 | 6,655.90 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold | 21.57 x 1e3(+0.69%)Baseline: 21.42 x 1e3 | 21.63 x 1e3 (99.69%) | 📈 view plot 🚷 view threshold | 606.00(0.00%)Baseline: 606.00 | 612.06 (99.01%) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1287 +/- ##
==========================================
+ Coverage 93.14% 93.33% +0.18%
==========================================
Files 123 124 +1
Lines 11007 11096 +89
==========================================
+ Hits 10253 10356 +103
+ Misses 754 740 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks, looks good to me! Happy to merge once we have a tket release with the extension
| # TODO: Shouldn't this be `to_hugr_poly` since it can contain | ||
| # a variable with a generic type? | ||
| hugr_ty = body_ty.to_hugr(ctx) |
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, this will most likely break, but afaik nested functions that reference type variables from the parent function also aren't handled correctly.
I guess the best solution for now would be to detect this case and emit an error telling the user that this isn't supported yet.
To properly handle this, we'll need to turn the modifier into a generic function and then instantiate it with the variables from the parent function...
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 it okay to merge this PR without such a proper error handling?
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, I think it's fine. We just have to wait for the tket release
| # Dummy variables to suppress Undefined name | ||
| # TODO: `ruff` fails when without these, which need to be fixed | ||
| dagger = object() | ||
| control = object() | ||
| power = object() |
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 think it would be good to add these to guppylang.std.lang and then reexport in guppylang.std.builtins.
But I'm also happy if you prefer to do this in a separate PR
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 forgot to mention, but I made an issue for this (#1290)
I would rather think it should be left as a future feature to be handled in a better manner.
🤖 I have created a release *beep* *boop* --- ## [0.25.0](guppylang-internals-v0.24.0...guppylang-internals-v0.25.0) (2025-10-28) ### ⚠ BREAKING CHANGES * (guppy-internals) Arrays are now lowered to `borrow_array`s instead of `value_array`s so elements do no longer need to be wrapped in options during lowering. * `checker.core.requires_monomorphization` renamed into `require_monomorphization` and now operating on all parameters simultaneously `tys.subst.BoundVarFinder` removed. Instead, use the new `bound_vars` property on types, arguments, and consts. `tys.parsing.parse_parameter` now requires a `param_var_mapping`. ### Features * compiler for modifiers ([#1287](#1287)) ([439ff1a](439ff1a)) * modifiers in CFG and its type checker (experimental) ([#1281](#1281)) ([fe85018](fe85018)) * Turn type parameters into dependent telescopes ([#1154](#1154)) ([b56e056](b56e056)) * update hugr, tket-exts and tket ([#1305](#1305)) ([6990d85](6990d85)) * Use `borrow_array` instead of `value_array` for array lowering ([#1166](#1166)) ([f9ef42b](f9ef42b)) ### Bug Fixes * compilation of affine-bounded type variables ([#1308](#1308)) ([49ecb49](49ecb49)) * Detect unsolved generic parameters even if they are unused ([#1279](#1279)) ([f830db0](f830db0)), closes [#1273](#1273) * Fix bug in symbolic pytket circuit loading with arrays ([#1302](#1302)) ([e6b90e8](e6b90e8)), closes [#1298](#1298) * Improve track_hugr_side_effects, adding Order edges from/to Input/Output ([#1311](#1311)) ([3c6ce7a](3c6ce7a)) * multiline loop arguments ([#1309](#1309)) ([836ef72](836ef72)) --- 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]>
This PR implements the Guppy compiler for modifiers.
Major changes
Issues
Future implementation