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: add results extensions #494

Merged
merged 7 commits into from
Jul 19, 2024
Merged

feat: add results extensions #494

merged 7 commits into from
Jul 19, 2024

Conversation

ss2165
Copy link
Member

@ss2165 ss2165 commented Jul 17, 2024

Closes #452

Closes #452

I'm not very pleased with this, but it does the job and all the "better" things I thought of didn't seem worth the extra code.

Very open to suggestions.
@ss2165 ss2165 requested a review from a team as a code owner July 17, 2024 14:33
@ss2165 ss2165 requested review from aborgna-q and doug-q and removed request for aborgna-q July 17, 2024 14:33
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 90.18868% with 26 lines in your changes missing coverage. Please review.

Project coverage is 81.53%. Comparing base (d247ffe) to head (a9ecaeb).
Report is 1 commits behind head on main.

Files Patch % Lines
tket2-hseries/src/extension/result.rs 90.18% 17 Missing and 9 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
- Coverage   82.50%   81.53%   -0.97%     
==========================================
  Files          63       47      -16     
  Lines        6299     6040     -259     
  Branches     5778     6040     +262     
==========================================
- Hits         5197     4925     -272     
- Misses        834      837       +3     
- Partials      268      278      +10     
Flag Coverage Δ
python ?
rust 81.53% <90.18%> (+0.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@doug-q doug-q left a comment

Choose a reason for hiding this comment

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

I've not looked carefully beyond the shape of the types, I agree they are not great but I don't have concrete(lol) ideas for improvement.

What happens when you merge BasicResultOp and ArrayResultOp into a single enum (say ResultOp, and then make struct ConcreteResultOp{ tag: string, def: ResultOp, typ: Type }.

lazy_static! {
/// The "tket2.result" extension.
pub static ref EXTENSION: Extension = {
let mut ext = Extension::new(EXTENSION_ID);
Copy link
Contributor

Choose a reason for hiding this comment

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

you should use new_with_reqs

}

#[derive(Clone, Copy, Debug, Serialize, Deserialize, Hash, PartialEq, Eq)]
enum ResultOpDef {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like OpDef in the name here. OpDef is an op before type args are applied, but this is used to capture the array length arg.

tket2-hseries/src/extension/result.rs Show resolved Hide resolved
tket2-hseries/src/extension/result.rs Show resolved Hide resolved
tket2-hseries/src/extension/result.rs Show resolved Hide resolved
@ss2165 ss2165 requested a review from doug-q July 18, 2024 12:45
@ss2165
Copy link
Member Author

ss2165 commented Jul 18, 2024

Thanks for your ideas @doug-q, I've implemented many of them. But I kept the single concrete op.

Copy link
Contributor

@doug-q doug-q left a comment

Choose a reason for hiding this comment

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

Nice.

I think you can construct your types more simply as noted inline.

You should probably implement MakeOpDef::description

tket2-hseries/src/extension/result.rs Show resolved Hide resolved
}

fn int_tv(int_tv_idx: usize) -> Type {
INT_EXTENSION
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use int_types::int_type(TypeArg::new_var_use(int_tv_idx, LOG_WIDTH_TYPE_PARAM)) here I think

}

fn array_type(inner_t: Type) -> Type {
let size_var = TypeArg::new_var_use(1, TypeParam::max_nat());
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use prelude::array_type(TypeArg::new_var_use(1, TypeParam::max_nat(), inner_t) here.

@ss2165 ss2165 requested a review from doug-q July 19, 2024 09:29
@ss2165 ss2165 added this pull request to the merge queue Jul 19, 2024
Merged via the queue into main with commit 034ab9c Jul 19, 2024
17 checks passed
@ss2165 ss2165 deleted the ss/results branch July 19, 2024 09:34
@hugrbot hugrbot mentioned this pull request Jul 18, 2024
@hugrbot hugrbot mentioned this pull request Aug 1, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 1, 2024
## 🤖 New release
* `tket2`: 0.1.0-alpha.2 -> 0.1.0
* `tket2-hseries`: 0.1.0

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

## `tket2`
<blockquote>

##
[0.1.0](tket2-v0.1.0-alpha.2...tket2-v0.1.0)
- 2024-08-01

### Bug Fixes
- Single source of truth for circuit names, and better circuit errors
([#390](#390))
- Support non-DFG circuits
([#391](#391))
- Portmatching not matching const edges
([#444](#444))
- Pattern matcher discriminating on opaqueOp description
([#441](#441))
- `extract_dfg` inserting the output node with an invalid child order
([#442](#442))
- Recompile ecc sets after
[#441](#441)
([#484](#484))

### Documentation
- Update tket2-py readme
([#431](#431))
- Better error reporting in portmatching
([#437](#437))
- Improved multi-threading docs for Badger
([#495](#495))

### New Features
- `Circuit::operations` ([#395](#395))
- tuple unpack rewrite ([#406](#406))
- guppy → pytket conversion
([#407](#407))
- Drop linear bits, improve pytket encoding/decoding
([#420](#420))
- *(py)* Allow using `Tk2Op`s in the builder
([#436](#436))
- Initial support for `TailLoop` as circuit parent
([#417](#417))
- Support tuple unpacking with multiple unpacks
([#470](#470))
- Partial tuple unpack ([#475](#475))
- [**breaking**] Compress binary ECCs using zlib
([#498](#498))
- Add timeout options and stats to Badger
([#496](#496))
- Expose advanced Badger timeout options to tket2-py
([#506](#506))

### Refactor
- [**breaking**] Simplify tket1 conversion errors
([#408](#408))
- Cleanup tket1 serialized op structures
([#419](#419))

### Testing
- Add coverage for Badger split circuit multi-threading
([#505](#505))
</blockquote>

## `tket2-hseries`
<blockquote>

##
[0.1.0](https://github.com/CQCL/tket2/releases/tag/tket2-hseries-v0.1.0)
- 2024-08-01

### New Features
- [**breaking**] init tket2-hseries
([#368](#368))
- *(tket2-hseries)* Add `tket2.futures` Hugr extension
([#471](#471))
- Add lazify-measure pass
([#482](#482))
- add results extensions
([#494](#494))
- *(tket2-hseries)* [**breaking**] Add `HSeriesPass`
([#487](#487))
</blockquote>


</p></details>

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

---------

Co-authored-by: Douglas Wilson <[email protected]>
@hugrbot hugrbot mentioned this pull request Aug 1, 2024
github-merge-queue bot pushed a commit to CQCL/guppylang that referenced this pull request Aug 12, 2024
Fixes #369 and fixes #365.

Make Guppy conform with the latest results spec introduced in
CQCL/tket2#494

BREAKING CHANGE:
* Result tags are now strings instead of ints
* Only numeric values and arrays thereof are allowed as results
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.

Implement "result" extension
2 participants