fix: use correct array ops #503
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
+ Coverage 91.54% 91.59% +0.04%
==========================================
Files 60 60
Lines 6130 6152 +22
==========================================
+ Hits 5612 5635 +23
+ Misses 518 517 -1 ☔ View full report in Codecov by Sentry. |
| @@ -50,21 +50,17 @@ class ArrayGetitemCompiler(CustomCallCompiler): | |||
| def build_classical_getitem( | |||
| self, | |||
| array: Wire, | |||
| array_ty: ht.Type, | |||
| array_ty: ht.Array, | |||
There was a problem hiding this comment.
in passing, it seems a bit redundant to pass both array_ty and elem_ty in at the same time. There seem to be about 5 different ways to get hold of the element type (all of which should give the same answer - type args to the array type, type args to the op, type of the input wire, .....)
There was a problem hiding this comment.
Yes indeed. I will change this back to Type, as it was before.
| conditional = builder.add_conditional(result) | ||
| with conditional.add_case(0) as case: | ||
| error = build_error(case, 1, "array get index out of bounds") | ||
| case.set_outputs(build_panic(case, [], [elem_ty], error)) |
There was a problem hiding this comment.
not *build_panic? Ah, it returns a Wire (rather than a singleton tuple/collection) if there is exactly one result type (even if that's passed in as a list rather than as a type)? I.e.
case.set_outputs(*build_panic(case, [], [elem_ty], error))
or
case.set_outputs(build_panic(case, [], elem_ty, error))
this may well be right, but does look a bit like there is something non-uniform about the handling of singleton elements by these various different APIs that we might want to come back to later.
There was a problem hiding this comment.
It's very confusing, figuring out to add the asterisk to the other callsite took me more than an hour. I will change this one.
There was a problem hiding this comment.
I think mypy is complaining about this too
There was a problem hiding this comment.
AFAICS set_outputs takes *Wire (i.e. varargs), so the elements need to be individual Wires not a list[Wire] - if you had a list[Wire] you would have to use *. That's in hugr repo.
However, build_panic (in guppy repo) claims to return a single Wire, so there shouldn't be any *ing of that (?!). And the body of build_panic returns builder.add_op(...), where add_op (in Hugr repo) claims to return a Node - so that doesn't look right, either.
So I'm not sure what's going on here!
There was a problem hiding this comment.
Ah, got it - your latest commit fixes things so they make sense and even mypy agrees. (build_panic returning Node means you can * that to get a series of Wires). Looks good now!
There are already tests for all of this. Changing from "UnsupportedOp" to an op from "prelude" does change the codepath, but it still passes. |
Can we do an execution test now? (If |
You're right Alan, I've added said test. |
🤖 I have created a release *beep* *boop* --- ## [0.12.0](v0.11.0...v0.12.0) (2024-09-18) ### ⚠ BREAKING CHANGES * Pytket circuits loaded via a `py` expression no longer take ownership of the passed qubits. * Lists and function tensors are no longer available by default. `guppylang.enable_experimental_features()` must be called before compilation to enable them. * The `GuppyModule` argument is now optional for all decorators and no longer the first positional argument. Removed the explicit module objects `builtins`, `quantum`, and `angle`. * `quantum_functional` is now its own Guppy module and no longer implicitly comes with `quantum`. * Linear function arguments are now borrowed by default; removed the now redundant `@inout` annotation ### Features * Add functions to quantum module and make quantum_functional independent ([#494](#494)) ([0b0b1af](0b0b1af)) * Hide lists and function tensors behind experimental flag ([#501](#501)) ([c867f48](c867f48)) * Make linear types [@inout](https://github.com/inout) by default; add [@owned](https://github.com/owned) annotation ([#486](#486)) ([e900c96](e900c96)) * Only lower definitions to Hugr if they are used ([#496](#496)) ([cc2c8a4](cc2c8a4)) * Support implicit modules for all decorators and turn builtins into implicit module ([#476](#476)) ([cc8a424](cc8a424)) * Use inout for pytket circuits ([#500](#500)) ([a980ec2](a980ec2)) ### Bug Fixes * `angle` is now a struct and emitted as a rotation ([#485](#485)) ([992b138](992b138)) * Evade false positives for inout variable usage ([#493](#493)) ([6fdb5d6](6fdb5d6)) * Fix redefinition of structs ([#499](#499)) ([0b156e9](0b156e9)) * Initialise _checked in GuppyModule ([#491](#491)) ([3dd5dd3](3dd5dd3)), closes [#489](#489) * use correct array ops ([#503](#503)) ([720d8b8](720d8b8)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
No description provided.