-
Notifications
You must be signed in to change notification settings - Fork 24
feat: added a new guppy data type: enum, with parsing and checking #1419
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
base: feat/guppy-enum
Are you sure you want to change the base?
Conversation
|
| Branch | nats/parse-enum-definitions |
| 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 | 143.70 x 1e3(+0.10%)Baseline: 143.55 x 1e3 | 144.98 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.67 x 1e3(0.00%)Baseline: 21.67 x 1e3 | 21.89 x 1e3 (99.01%) | 📈 view plot 🚷 view threshold | 606.00(0.00%)Baseline: 606.00 | 612.06 (99.01%) |
|
mypy is failing due to the dummy implementation of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/guppy-enum #1419 +/- ##
===================================================
- Coverage 93.56% 93.46% -0.11%
===================================================
Files 128 129 +1
Lines 11476 11662 +186
===================================================
+ Hits 10738 10900 +162
- Misses 738 762 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tatiana-s
left a comment
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.
Great work! Some mostly minor comments, otherwise do you think there is any way to make the dummy methods pass mypy so that the rest of CI can run?
guppylang-internals/src/guppylang_internals/checker/errors/generic.py
Outdated
Show resolved
Hide resolved
…py:125 Co-authored-by: tatiana-s <tatiana.sedelnikov@quantinuum.com>
Done 👍 |
Co-authored-by: tatiana-s <tatiana.sedelnikov@quantinuum.com>
https://github.com/Quantinuum/guppylang/pull/1419/files/6df0339b84e379ed591a1cd7e89b9b587e2ce9df#r2664703962 Co-authored-by: tatiana-s <tatiana.sedelnikov@quantinuum.com>
tatiana-s
left a comment
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.
Some naming suggestions otherwise fine to merge into the feature branch I think 👍
guppylang-internals/src/guppylang_internals/definition/struct.py
Outdated
Show resolved
Hide resolved
mark-koch
left a comment
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.
Looks good! Just leaving a couple of smaller suggestions
Also, can you mark this PR as breaking (for guppylang_internals) and list all your refactor changes as BREAKING CHANGE?
| # need to store and check if any dependencies have changed. | ||
| self.reset() | ||
|
|
||
| self.to_check_worklist[id] = self.get_parsed(id) |
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.
Can you update the branch to pull in the merged #1410 instead of inlining the fix here
| """Chechks instantiation of this enum type.""" | ||
|
|
||
| def generated_methods(self) -> list[CustomFunctionDef]: | ||
| # Generating methods to instantiate enum variants |
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.
Sould this be a TODO? If yes, can you add it to the tracking issue?
| # TODO: Considering renaming to UncheckedField and CheckedField, | ||
| # and joining with UncheckedStructField and StructField |
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 this would be a good idea 👍
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class EnumVariant: |
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.
You could also consider merging the variant classes:
F = TypeVar("F", UncheckedField, CheckedField)
@dataclass(frozen=True)
class EnumVariant(Generic[F]):
name: str
fields: Sequence[F]and then use EnumVariant[UncheckedField] and EnumVariant[CheckedField]?
|
|
||
| defined_at: ast.ClassDef | ||
| params: Sequence[Parameter] | ||
| variants: Mapping[str, UncheckedEnumVariant] |
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.
Beware: We will need a unique ordering of enum variants when converting them to Hugr sum types. Now, technically this will be fine since Python dicts are insertion ordered, but it's a bit of a hidden invariant.
Maybe good to add a comment here, that the insertion order of variants determines the index of the variant? Or maybe even store the index in the variant class?
Co-authored-by: Mark Koch <48097969+mark-koch@users.noreply.github.com>
Co-authored-by: Mark Koch <48097969+mark-koch@users.noreply.github.com>
…uppylang into nats/parse-enum-definitions
guppylang_internals/definition/enum.pycontainingRawEnumDef,ParsedEnumDef, andCheckedEnumDef(WIP)guppylang_internals/definition/util.pymodule with common function and classes used byenum.py,struct.pyandfunction.py