-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Load pytket
circuit as a function definition
#672
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #672 +/- ##
==========================================
- Coverage 92.75% 92.63% -0.13%
==========================================
Files 68 69 +1
Lines 7720 7833 +113
==========================================
+ Hits 7161 7256 +95
- Misses 559 577 +18 ☔ View full report in Codecov by Sentry. |
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! Mostly just some nits and suggestions.
It looks like the coverage report doesn't include the test you've written for some reason. It should be running with tket2 installed, so no idea what's going on there.
Speaking of tests, could you also add some error tests for every GuppyError
that is thrown. See the tests/error
directory
# TODO: Error if hugr isn't FuncDefn? | ||
if node_data and isinstance(node_data.op, FuncDefn): |
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, we should throw an InternalGuppyError
input_types.remove(Bool) | ||
|
||
else: | ||
raise GuppyError(PytketNotCircuit(self.defined_at)) |
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'd prefer to just raise this as a Python TypeError
in guppy.pytket
. No need to wait until compilation to throw this error
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
# Negative index slicing doesn't work when num_returns is 0. | ||
if num_returns == 0: | ||
return CallReturnWires( | ||
regular_returns=[], | ||
inout_returns=list(call[0:]), | ||
) | ||
else: | ||
# Circuit function returns are the other way round to Guppy returns. | ||
return CallReturnWires( | ||
regular_returns=list(call[-num_returns:]), | ||
inout_returns=list(call[:-num_returns]), | ||
) |
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.
Consider doing
# Negative index slicing doesn't work when num_returns is 0. | |
if num_returns == 0: | |
return CallReturnWires( | |
regular_returns=[], | |
inout_returns=list(call[0:]), | |
) | |
else: | |
# Circuit function returns are the other way round to Guppy returns. | |
return CallReturnWires( | |
regular_returns=list(call[-num_returns:]), | |
inout_returns=list(call[:-num_returns]), | |
) | |
num_inouts = len(list(call)) - num_returns | |
return CallReturnWires( | |
regular_returns=list(call[inout_returns:]), | |
inout_returns=list(call[:inout_returns]), | |
) |
to avoid the 0 edge case
def foo(q: qubit) -> None: | ||
result = guppy_circ(q) |
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.
def foo(q: qubit) -> None: | |
result = guppy_circ(q) | |
def foo(q: qubit) -> bool: | |
return guppy_circ(q) |
This way, validation checks that the bool is wired correctly
The reason coverage is not taking the tests into account is because |
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, thank you!
🤖 I have created a release *beep* *boop* --- ## [0.14.0](v0.13.1...v0.14.0) (2024-12-19) ### ⚠ BREAKING CHANGES * Lists in `py(...)` expressions are now turned into Guppy arrays instead of lists. * `dirty_qubit` function removed * measure_return renamed to `project_z` ### Features * add `maybe_qubit` stdlib function ([#705](#705)) ([a49f70e](a49f70e)), closes [#627](#627) * add measure_array and discard_array quantum function ([#710](#710)) ([3ad49ff](3ad49ff)) * Add method to load pytket circuit without function stub ([#712](#712)) ([ee1e3de](ee1e3de)) * Add Option type to standard library ([#696](#696)) ([45ea6b7](45ea6b7)) * Allow generic nat args in statically sized ranges ([#706](#706)) ([f441bb8](f441bb8)), closes [#663](#663) * Array comprehension ([#613](#613)) ([fdc0526](fdc0526)), closes [#614](#614) [#616](#616) [#612](#612) * Implicit coercion of numeric types ([#702](#702)) ([df4745b](df4745b)), closes [#701](#701) * Load `pytket` circuit as a function definition ([#672](#672)) ([b21b7e1](b21b7e1)) * Make arrays iterable ([#632](#632)) ([07b9871](07b9871)) * qsystem std functions with updated primitives ([#679](#679)) ([b0f041f](b0f041f)) * remove dirty_qubit ([#698](#698)) ([78e366b](78e366b)) * Turn py expression lists into arrays ([#697](#697)) ([d52a00a](d52a00a)) * Unpacking assignment of iterable types with static size ([#688](#688)) ([602e243](602e243)) * update to hugr 0.10 and tket2 0.6 ([#725](#725)) ([63ea7a7](63ea7a7)) ### Bug Fixes * Accept non-negative int literals and py expressions as nats ([#708](#708)) ([a93d4fe](a93d4fe)), closes [#704](#704) * Allow borrowing inside comprehensions ([#723](#723)) ([02b6ab0](02b6ab0)), closes [#719](#719) * Detect unsupported default arguments ([#659](#659)) ([94ac7e3](94ac7e3)), closes [#658](#658) * docs build command ([#729](#729)) ([471b74c](471b74c)) * Ensure `int`s can be treated as booleans ([#709](#709)) ([6ef6d60](6ef6d60)), closes [#681](#681) * Fix array execution bugs ([#731](#731)) ([0f6ceaa](0f6ceaa)) * Fix implicit modules in IPython shells ([#662](#662)) ([4ecb5f2](4ecb5f2)), closes [#661](#661) * Properly report error for unsupported constants ([#724](#724)) ([d0c2da4](d0c2da4)), closes [#721](#721) * Properly report errors for unsupported expressions ([#692](#692)) ([7f24264](7f24264)), closes [#691](#691) * remove use of deprecated Ellipsis ([#699](#699)) ([b819a84](b819a84)) ### Documentation * Fix docs build ([#700](#700)) ([684f485](684f485)), closes [#680](#680) * fix README.md and quickstart.md ([#654](#654)) ([abb0221](abb0221)) --- 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]>
Closes #657, however the following limitations still exist which are covered by further issues:
pytket
circuit functions #669:Classical bits and measurements aren't currently supported as this requires some checks related to ownership and return values.They are supported but without taking ownership.pytket
functions without explicit function stub #670: It would be nice to not have to provide an explicit function stub for the circuit and also haveload_pytket
method which infers it.pytket
function stubs #671: It would also be nice to be able to provide arrays of qubits in the function stub.