-
Notifications
You must be signed in to change notification settings - Fork 13
feat: Type of constants in core Terms.
#2411
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
Conversation
core Terms.core Terms.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2411 +/- ##
=======================================
Coverage 82.66% 82.66%
=======================================
Files 251 251
Lines 46599 46644 +45
Branches 42160 42190 +30
=======================================
+ Hits 38521 38559 +38
- Misses 6028 6034 +6
- Partials 2050 2051 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3962e45 to
2c329cd
Compare
675c2ca to
b9acbff
Compare
acl-cqc
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.
Generally fine, a couple of suggestions - naming?
And, it's a bit of a shocker to have only 35% coverage! Obviously one cannot get very far without any terms of type Term::Const(....) but I think at the least you could add some roundtrip serialization tests and things like that?
| #[display("{_0}")] | ||
| Variable(TermVar), | ||
|
|
||
| /// The type of constants for a runtime type. |
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 type of constants for a runtime type. | |
| /// The type of constants, i.e. terms that can be turned into runtime values of a given (runtime) type. |
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.
Noting your example of qubit, I think we definitely want to say something stronger here - these are not static representations of the runtime values; they can (effectively) be arbitrary recipes or factories, procedures for production of.
(I think it's ok to continue to call them ConstType given "constants" are the common usage so long as we doc that really this is a more general concept than a constant)
(For example, there would not necessarily even be a guarantee that two runtime values produced by the same recipe would be equal, would there? I mean, there is not necessarily a notion of equality...but if there was!)
hugr-core/src/types/type_param.rs
Outdated
| Variable(TermVar), | ||
|
|
||
| /// The type of constants for a runtime type. | ||
| Const(Box<Type>), |
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 reckon this should be called ConstType, paralleling BytesType or RuntimeType. (In hindsight, using a Param suffix, rather than ...Type, would have been more similar to the python!).
There may never be a Const constructor (i.e. in the family of TypeArgs, a term that is/produces a runtime value) - rather, there will likely be a number of different ones, but I think we can still try to be consistent wrt. naming.
| (Term::TupleType(_), Term::StaticType) => Ok(()), | ||
| (Term::RuntimeType(_), Term::StaticType) => Ok(()), | ||
| (Term::Const(_), Term::StaticType) => Ok(()), | ||
|
|
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 guess we do not want to say that e.g. Term::BoundedNat is a const (e.g. of Runtime type usize) but rather wait for a different term that does that?
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.
Indeed! A Term::BoundedNat is a compile time value. It can be used as a parameter to a custom term that describes how to produce a particular type of runtime integer from it.
| "(Linear, Nat(3))", | ||
| ), | ||
| (ListParam(StringParam()), "[String]"), | ||
| (ConstParam(Qubit), "Const(Qubit)"), |
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'm sure this is valid** but it's a bit misleading, we are never going to have a Term that produces a Qubit "constant" are we?
** this is why we said constants have to be Copyable in hugr-core; I still feel it is a shame not to be able to here
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.
Glad that I randomly picked that example then! Constants are descriptions on how to produce runtime values, so we can certainly have constants that produce qubits. Loading such a constant would mean preparing a qubit in a particular state.
(declare-ctr quantum.const_zero (core.const quantum.qubit))
(declare-ctr quantum.const_one (core.const quantum.qubit))
(declare-ctr quantum.const_plus (core.const quantum.qubit))
(declare-ctr quantum.const_minus (core.const quantum.qubit))Note that the constant is always copyable since it is only the blueprint. The result of loading the constant need not be copyable. This is one of the advantages that are afforded by this system of constants.
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.
Ok, indeed good you picked that one :). Let's be more explicit in the doc-comments on declaration of ConstType (I've commented there too), then.
|
Yeah, will add some roundtrip tests. I am looking forward to the day when we have custom terms so that PRs like this will become trivial. |
0585551 to
402cc24
Compare
acl-cqc
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.
Thanks @zrho, sorry for dropping the ball. Nice! :) Just see comments on the doc for ConstType.
| #[display("{_0}")] | ||
| Variable(TermVar), | ||
|
|
||
| /// The type of constants for a runtime type. |
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.
Noting your example of qubit, I think we definitely want to say something stronger here - these are not static representations of the runtime values; they can (effectively) be arbitrary recipes or factories, procedures for production of.
(I think it's ok to continue to call them ConstType given "constants" are the common usage so long as we doc that really this is a more general concept than a constant)
(For example, there would not necessarily even be a guarantee that two runtime values produced by the same recipe would be equal, would there? I mean, there is not necessarily a notion of equality...but if there was!)
hugr-core/src/types/type_param.rs
Outdated
|
|
||
| /// The type of constants for a runtime type. | ||
| /// | ||
| /// A constant is a compile time description of a runtime value. The runtime |
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.
So here - it does not describe a runtime value - maybe a population of them (e.g. floats produced according to a distribution, right?) but it's weak and/or presumptive to say that the constant describes an (individual) value coming from it.
(You also can't go backwards, from runtime value to constant, but that's a side-issue)
| Self::TupleType(Box::new(item_types.into())) | ||
| } | ||
|
|
||
| /// Creates a new [`Term::ConstType`] from a runtime type. |
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.
nit: perhaps
| /// Creates a new [`Term::ConstType`] from a runtime type. | |
| /// Creates a new [`Term::ConstType`] for a runtime type. |
|
|
||
| mod.declare_function( | ||
| "const_type", | ||
| tys.PolyFuncType( |
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.
right - nice example - statically parametric over a recipe for qubits, at runtime just produces qubits. Just to be clear - this is not like the one in model__roundtrip_const.snap, where the function's output is the constant (recipe) not the runtime type produced by it.
| public | ||
| example.const_as_param | ||
| (param ?0 (core.const arithmetic.float.types.float64)) | ||
| (core.fn [] [?0])) |
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.
Just to be clear - not sure if you can have a comment here or in model-const.edn? - the function here returns the constant recipe, not any runtime float....right?
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.
Good catch, this is a type error. It should return the float.
| "(Linear, Nat(3))", | ||
| ), | ||
| (ListParam(StringParam()), "[String]"), | ||
| (ConstParam(Qubit), "Const(Qubit)"), |
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.
Ok, indeed good you picked that one :). Let's be more explicit in the doc-comments on declaration of ConstType (I've commented there too), then.
## 🤖 New release
* `hugr-model`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-core`: 0.22.1 -> 0.23.0 (⚠ API breaking changes)
* `hugr-llvm`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-passes`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-persistent`: 0.2.1 -> 0.2.2 (✓ API compatible changes)
* `hugr`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
* `hugr-cli`: 0.22.1 -> 0.23.0 (✓ API compatible changes)
### ⚠ `hugr-core` breaking changes
```text
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/struct_missing.ron
Failed in:
struct hugr_core::hugr::views::sibling_subgraph::TopoConvexChecker, previously in file /tmp/.tmpA42bjc/hugr-core/src/hugr/views/sibling_subgraph.rs:601
```
<details><summary><i><b>Changelog</b></i></summary><p>
## `hugr-model`
<blockquote>
##
[0.23.0](hugr-model-v0.22.1...hugr-model-v0.23.0)
- 2025-08-06
### New Features
- Type of constants in `core` `Term`s.
([#2411](#2411))
</blockquote>
## `hugr-core`
<blockquote>
##
[0.23.0](hugr-core-v0.22.1...hugr-core-v0.23.0)
- 2025-08-06
### New Features
- Type of constants in `core` `Term`s.
([#2411](#2411))
- Support LineConvexChecker
([#2487](#2487))
</blockquote>
## `hugr-llvm`
<blockquote>
##
[0.23.0](hugr-llvm-v0.22.1...hugr-llvm-v0.23.0)
- 2025-08-06
### Bug Fixes
- added public func getter for EmitFuncContext
([#2482](#2482))
- *(hugr-llvm)* Set llvm function linkage based on Visibility hugr node
field ([#2502](#2502))
</blockquote>
## `hugr-passes`
<blockquote>
##
[0.22.1](hugr-passes-v0.22.0...hugr-passes-v0.22.1)
- 2025-07-28
### New Features
- Include copy_discard_array in DelegatingLinearizer::default
([#2479](#2479))
- Inline calls to functions not on cycles in the call graph
([#2450](#2450))
</blockquote>
## `hugr-persistent`
<blockquote>
##
[0.2.0](hugr-persistent-v0.1.0...hugr-persistent-v0.2.0)
- 2025-07-24
### New Features
- [**breaking**] Update portgraph dependency to 0.15
([#2455](#2455))
</blockquote>
## `hugr`
<blockquote>
##
[0.23.0](hugr-v0.22.1...hugr-v0.23.0)
- 2025-08-06
### New Features
- Type of constants in `core` `Term`s.
([#2411](#2411))
- Support LineConvexChecker
([#2487](#2487))
</blockquote>
## `hugr-cli`
<blockquote>
##
[0.23.0](hugr-cli-v0.22.1...hugr-cli-v0.23.0)
- 2025-08-06
### New Features
- *(cli)* Deprecate HugrInputArgs::get_hugr
([#2506](#2506))
</blockquote>
</p></details>
---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
---------
Co-authored-by: Luca Mondada <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [0.13.1](hugr-py-v0.13.0...hugr-py-v0.13.1) (2025-08-18) ### Features * Add option to render a subgraph of a hugr. ([#2497](#2497)) ([c3370fe](c3370fe)) * **py:** implement `Sequence` protocol for qsys results/shots ([#2524](#2524)) ([b42e9df](b42e9df)) * Type of constants in `core` `Term`s. ([#2411](#2411)) ([2ba5764](2ba5764)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
## 🤖 New release * `hugr-model`: 0.22.2 -> 0.22.3 (✓ API compatible changes) * `hugr-core`: 0.22.2 -> 0.22.3 (✓ API compatible changes) * `hugr-llvm`: 0.22.2 -> 0.22.3 (✓ API compatible changes) * `hugr-passes`: 0.22.2 -> 0.22.3 (✓ API compatible changes) * `hugr-persistent`: 0.2.2 -> 0.2.3 (✓ API compatible changes) * `hugr`: 0.22.2 -> 0.22.3 (✓ API compatible changes) * `hugr-cli`: 0.22.2 -> 0.22.3 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `hugr-model` <blockquote> ## [0.22.2](hugr-model-v0.22.1...hugr-model-v0.22.2) - 2025-08-06 ### New Features - Type of constants in `core` `Term`s. ([#2411](#2411)) </blockquote> ## `hugr-core` <blockquote> ## [0.22.3](hugr-core-v0.22.2...hugr-core-v0.22.3) - 2025-09-11 ### Bug Fixes - SiblingSubgraph::try_from_nodes not including disconnected components ([#2549](#2549)) ### Documentation - Clarify docs for SiblingSubgraph::{inputs, outputs} ([#2508](#2508)) ### New Features - SiblingSubgraph supports function calls ([#2528](#2528)) - Add unchecked constructor for SiblingSubgraph ([#2526](#2526)) - Add HugrMut::insert(_view)_forest ([#2518](#2518)) - Add extend_inputs function for DFGs ([#2536](#2536)) - Loosen bound on Patch trait ([#2545](#2545)) </blockquote> ## `hugr-llvm` <blockquote> ## [0.22.2](hugr-llvm-v0.22.1...hugr-llvm-v0.22.2) - 2025-08-06 ### Bug Fixes - added public func getter for EmitFuncContext ([#2482](#2482)) - *(hugr-llvm)* Set llvm function linkage based on Visibility hugr node field ([#2502](#2502)) </blockquote> ## `hugr-passes` <blockquote> ## [0.22.3](hugr-passes-v0.22.2...hugr-passes-v0.22.3) - 2025-09-11 ### New Features - SiblingSubgraph supports function calls ([#2528](#2528)) </blockquote> ## `hugr-persistent` <blockquote> ## [0.2.3](hugr-persistent-v0.2.2...hugr-persistent-v0.2.3) - 2025-09-11 ### Documentation - Clarify docs for SiblingSubgraph::{inputs, outputs} ([#2508](#2508)) ### New Features - SiblingSubgraph supports function calls ([#2528](#2528)) </blockquote> ## `hugr` <blockquote> ## [0.22.3](hugr-v0.22.2...hugr-v0.22.3) - 2025-09-11 ### Bug Fixes - SiblingSubgraph::try_from_nodes not including disconnected components ([#2549](#2549)) ### Documentation - Clarify docs for SiblingSubgraph::{inputs, outputs} ([#2508](#2508)) ### New Features - SiblingSubgraph supports function calls ([#2528](#2528)) - Add unchecked constructor for SiblingSubgraph ([#2526](#2526)) - Add extend_inputs function for DFGs ([#2536](#2536)) - Loosen bound on Patch trait ([#2545](#2545)) - Add HugrMut::insert(_view)_forest ([#2518](#2518)) </blockquote> ## `hugr-cli` <blockquote> ## [0.22.3](hugr-cli-v0.22.2...hugr-cli-v0.22.3) - 2025-09-11 ### New Features - *(hugr-cli)* CliError::validate helper ([#2507](#2507)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
This PR adds a
Term::Constvariant, representing the type of constants for a given runtime type. This corresponds to thecore.consttype inhugr-model.Closes #2304.