-
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
fix: Fix generic array functions #630
Conversation
# See https://github.com/CQCL/guppylang/issues/631 | ||
raise GuppyError("Array results are currently disabled", 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.
The change in this PR breaks array results. We could emit code to unwrap the array elements, but imo it's now worth it since this will be undone once we do #628.
What do you think?
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 breaks array results because those ops take an Array<N,f64>
, not a Array<N,Option<f64>>
.
This is certainly a shame, but the lowering isn't done so it's surely tolerable.
I am not sure enough of exactly what 628 will entail to be confident that that it will be undone then.
Your could emit HUGR that converted an Array<N,Option<_>>
-> Array<N,_>
. That would be better I think.
I am with things as is, up to you.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/generic-defs #630 +/- ##
=====================================================
- Coverage 91.83% 91.68% -0.15%
=====================================================
Files 60 60
Lines 6465 6470 +5
=====================================================
- Hits 5937 5932 -5
- Misses 528 538 +10 ☔ View full report in Codecov by Sentry. |
# See https://github.com/CQCL/guppylang/issues/631 | ||
raise GuppyError("Array results are currently disabled", 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.
I think it breaks array results because those ops take an Array<N,f64>
, not a Array<N,Option<f64>>
.
This is certainly a shame, but the lowering isn't done so it's surely tolerable.
I am not sure enough of exactly what 628 will entail to be confident that that it will be undone then.
Your could emit HUGR that converted an Array<N,Option<_>>
-> Array<N,_>
. That would be better I think.
I am with things as is, up to you.
🤖 I have created a release *beep* *boop* --- ## [0.13.1](v0.13.0...v0.13.1) (2024-11-15) ### Features * Generic function definitions ([#618](#618)) ([7519b90](7519b90)), closes [#522](#522) * mem_swap function for swapping two inout values ([#653](#653)) ([89e10a5](89e10a5)) ### Bug Fixes * Fix generic array functions ([#630](#630)) ([f4e5655](f4e5655)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
See #629 for context.
This PR changes the lowering of classical arrays to use the same
Option
trick that we use for linear arrays. This fixes #629 in the short-term. Longer-term we should do #628 to address the problem properly