Skip to content
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: bindings for circuit cost and hash #252

Merged
merged 11 commits into from
Nov 23, 2023
Merged

Conversation

aborgna-q
Copy link
Collaborator

  • Adds Tk2Circuit::hash to the python bindings.
  • Adds bindings for computing the cost of a circuit.
  • Adds tests
    • I split test_bindings.py into test_circuit and test_pass

@aborgna-q aborgna-q requested a review from acl-cqc November 20, 2023 15:42
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (60c6608) 82.65% compared to head (66eb31c) 82.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #252      +/-   ##
==========================================
+ Coverage   82.65%   82.79%   +0.13%     
==========================================
  Files          34       34              
  Lines        4250     4267      +17     
  Branches     4151     4151              
==========================================
+ Hits         3513     3533      +20     
+ Misses        550      548       -2     
+ Partials      187      186       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aborgna-q aborgna-q requested review from ss2165 and removed request for acl-cqc November 21, 2023 15:30
Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I didn't realize "bindings" meant python, which in hindsight I should have. Definitely not feeling very expert here so maybe it's best that you've reassigned this one, but a couple of thoughts....

tket2-py/src/circuit/convert.rs Outdated Show resolved Hide resolved
/// :returns: The sum of all operation costs.
pub fn circuit_cost<'py>(&self, cost_fn: &'py PyAny) -> PyResult<&'py PyAny> {
let py = cost_fn.py();
let circ_cost = self.hugr.circuit_cost(|op| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some type annotations would help here. What's the result type of this lambda?

Copy link
Collaborator Author

@aborgna-q aborgna-q Nov 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned it up a bit. Hopefully it's better now.

use tket_json_rs::circuit_json::SerialCircuit;

use crate::rewrite::PyCircuitRewrite;

use super::PyCircuitCost;

/// A manager for tket 2 operations on a tket 1 Circuit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment really accurate? It stores a tket2 circuit i.e. Hugr, so doesn't introduce any new way of performing tket2 operations. Something like "that converts tket1 circuits into tket2 circuits/Hugrs in order to operate on them"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated comment. I replaced with a better explanation and a short example.

tket2-py/src/circuit/cost.rs Outdated Show resolved Hide resolved
tket2-py/src/circuit/cost.rs Outdated Show resolved Hide resolved
type Output = PyCircuitCost;

fn add(self, rhs: PyCircuitCost) -> Self::Output {
Python::with_gil(|py| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reading all of these impls with all of these with_gil and thinking - this would be so much easier if we had a wrapper on the python side and did the summing there. Indeed, probably also the mapping there. I guess that's not the current setup but if we are likely to head that way then it might save us a lot of code to just do that now....

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's annoying, but we're limited by the design. Once we have bindings for the hugr Walker we could revisit this.

@aborgna-q aborgna-q requested a review from acl-cqc November 21, 2023 16:53

/// Copy the circuit.
pub fn __copy__(&self) -> PyResult<Self> {
Ok(self.clone())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOI is it even possible to do something like the python shallow copy here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, unless you add Arcs everywhere.

tket2-py/src/circuit/convert.rs Outdated Show resolved Hide resolved
tket2-py/src/circuit/cost.rs Outdated Show resolved Hide resolved
@aborgna-q aborgna-q enabled auto-merge November 23, 2023 13:48
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@aborgna-q aborgna-q added this pull request to the merge queue Nov 23, 2023
Merged via the queue into main with commit 85ce5f9 Nov 23, 2023
11 checks passed
@aborgna-q aborgna-q deleted the feat/bindings-cost-hash branch November 23, 2023 14:02
ss2165 pushed a commit that referenced this pull request Dec 8, 2023
- Adds `Tk2Circuit::hash` to the python bindings.
- Adds bindings for computing the cost of a circuit.
- Adds tests
  - I split `test_bindings.py` into `test_circuit` and `test_pass`
github-merge-queue bot pushed a commit that referenced this pull request May 24, 2024
## 🤖 New release
* `tket2`: 0.1.0-alpha.1

<details><summary><i><b>Changelog</b></i></summary><p>

## `tket2`
<blockquote>

##
[0.0.0-alpha.1](https://github.com/CQCL/tket2/releases/tag/tket2-v0.0.0-alpha.1)
- 2024-05-24

### Bug Fixes
- Check for rewrite composition in badger
([#255](#255))
- induced cycles in depth optimisation
([#264](#264))
- Encode opaque symbolic constants
([#273](#273))
- Correctly detect custom ops by name
([#281](#281))
- Track input linear units in `Command`
([#310](#310))
- Don't convert tket2 measurements into tket1 ops
([#331](#331))

### Documentation
- Expand the main module and README docs
([#298](#298))

### New Features
- add angle type to tket2 extension
([#231](#231))
- bindings for circuit cost and hash
([#252](#252))
- Implement `PyErr` conversion locally in `tket2-py`
([#258](#258))
- Add a "progress timeout" to badger
([#259](#259))
- [**breaking**] Add lexicographic cost
([#270](#270))
- rewrite tracing ([#267](#267))
- Move pre/post rewrite cost to the RewriteStrategy API
([#276](#276))
- [**breaking**] Lexicographic cost fn
([#277](#277))
- Return rewrite strategies as a generator
([#275](#275))
- add qalloc, qfree, reset ops
([#284](#284))
- [**breaking**] Support any ops in portmatching
([#293](#293))
- Add `PatternMatch::nodes` and `subcircuit` + matching example
([#299](#299))
- Use `IncomingPort` and `OutgoingPort` instead of `Port` where
possible. ([#296](#296))
- expose Tk2Op name ([#307](#307))

### Refactor
- Move tket2 code to a workspace member
([#210](#210))
- Restructure the python code
([#211](#211))
- s/taso/badger/ ([#228](#228))
- Move python bindings from `tket2` to `tket2-py`
([#235](#235))
- rename t2op ([#256](#256))

### Testing
- Add small parallel badger test
([#237](#237))
- fix non-deterministic badger test
([#245](#245))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Agustin Borgna <agustin.borgna@quantinuum.com>
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2024
🤖 I have created a release *beep* *boop*
---


## 0.1.0 (2024-06-28)


### ⚠ BREAKING CHANGES

* require `hugr-0.5.0`
* Replaced `tket2.circuit.OpConvertError` with
`tket2.circuit.TK1ConvertError` in the python lib.
* Moved `tket2::json` to `tket2::serialize::pytket`
* Replaced the `Circuit` trait with a wrapper struct.
* This is a breaking change to the compiled rewriter serialisation
format.

### Features

* Add a "progress timeout" to badger
([#259](#259))
([556cf64](556cf64))
* Add missing typing hints
([#352](#352))
([4990613](4990613))
* bindings for circuit cost and hash
([#252](#252))
([85ce5f9](85ce5f9))
* drop pyo3 core dep ([#355](#355))
([9f7d415](9f7d415))
* EccRewriter bindings
([#251](#251))
([97e2e0a](97e2e0a))
* guppy → pytket conversion
([#407](#407))
([8c5a487](8c5a487))
* Implement `PyErr` conversion locally in `tket2-py`
([#258](#258))
([3e1a68d](3e1a68d))
* init tket2-hseries ([#368](#368))
([61e7535](61e7535))
* pauli propagation use case example
([#333](#333))
([f46973c](f46973c))
* **py:** Allow using `Tk2Op`s in the builder
([#436](#436))
([aed8651](aed8651))
* Support any ops in portmatching
([#293](#293))
([6b05a05](6b05a05))
* **tket2-py:** Bind the `lower_to_pytket` pass in python
([#439](#439))
([8208324](8208324))
* Use tket1 and tket2 circuits interchangeably everywhere
([#243](#243))
([eac7acf](eac7acf))
* Utilities for loading compiled guppy circuits
([#393](#393))
([028779a](028779a))


### Bug Fixes

* failed importlib import
([#254](#254))
([b077660](b077660))
* induced cycles in depth optimisation
([#264](#264))
([68c9ff2](68c9ff2)),
closes [#253](#253)
* Make native py modules behave like python's
([#212](#212))
([4220038](4220038)),
closes [#209](#209)
* pytest failing to find `tket2` in CI
([#367](#367))
([a9df8e6](a9df8e6))
* **tket2-py:** Replace `with_hugr` helpers with `with_circ`, keeping
the circuit parent. ([#438](#438))
([b77b3cb](b77b3cb))


### Documentation

* Add some example notebooks for the python package.
([#443](#443))
([4ed276c](4ed276c)),
closes [#434](#434)
* Update tket2-py readme
([6c8f18a](6c8f18a))


### Code Refactoring

* Rename `tket2::json` into `tket2::serialize::pytket`
([#392](#392))
([93e611c](93e611c))
* Replace Circuit trait with a struct
([#370](#370))
([ec5dd22](ec5dd22))
* Simplify tket1 conversion errors
([#408](#408))
([b0b8aff](b0b8aff))

---
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: Agustin Borgna <agustin.borgna@quantinuum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants