search

CQCL / hugr

Hierarchical Unified Graph Representation
https://crates.io/crates/hugr
Apache License 2.0
15 stars 4 forks source link

feat: Helper functions for requesting inference, use with builder in tests #1219

Closed acl-cqc closed 1 week ago

acl-cqc commented 1 week ago
acl-cqc commented 1 week ago

On #1195, @ss2165 wrote:

Thoughts on builder interface:

  • Builders that infer extensions should be default, therefore they should have the short names (probably just the set of names we currently use with explicit deltas).
  • Explicit delta versions, if necessary, can have some standard naming convention (e.g. dfg_builder_delta)
  • Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?
acl-cqc commented 1 week ago

On #1195, @ss2165 wrote:

Thoughts on builder interface:

  • Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?

On the latter - yes: every time a FunctionType is used inside a Type::new_function, say, or for a FuncDefn (which we don't infer, but I suppose could - just requires a more complex algorithm that builds a call graph and looks for cycles), or a FuncDecl (which we definitely can't infer). Also not for OpDef signatures...

Moreover whilst I agree that the builder should default to inference I'm less sure about Hugr's built directly without the builder. One thing we could do though would be

However whilst fine for directly-constructed Hugrs (inference is available at reduced cost but still slightly more than being explicit, which feels right) this still leaves the builder wanting.

I also thought about making the builder methods take not a FunctionType but an impl Into<FTSpec> (for some new type FTSpec) where we impl From<FunctionType> for FTSpec (providing all the existing interfaces from before this PR), but then we can also have impl From<(TypeRow, TypeRow)> for FTSpec (with extensions TO_BE_INFERRED) and impl From<TypeRow> for FTSpec (endomorphic TO_BE_INFERRED). However the problem with this is that we rely heavily (e.g. in FunctionType::new) on impl Into<TypeRow> with all sorts of handy conversions there (e.g. from a single Type or Vec<Type>) which we'd not be able to use. (I think the Rust foreign-trait impl rule prevents us from defining transitive conversions - we can't have impl <T: Into<TypeRow>> From<T> for FTSpec as we might like.)

Hence I think specific builder functions are the right way to go...

acl-cqc commented 1 week ago

@ss2165 sending this your way because you looked at the original (#1195) from which this was split. Feel free to reassign....

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.98%. Comparing base (c05edd3) to head (d4d1c4e).

Files Patch % Lines
hugr-core/src/builder/dataflow.rs 0.00% 0 Missing and 3 partials :warning:
hugr-core/src/hugr/rewrite/inline_dfg.rs 0.00% 0 Missing and 3 partials :warning:
hugr-core/src/hugr/validate/test.rs 75.00% 0 Missing and 1 partial :warning:
hugr-core/src/ops/constant.rs 75.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1219 +/- ## ========================================== - Coverage 86.99% 86.98% -0.02% ========================================== Files 100 100 Lines 18975 18948 -27 Branches 16992 16965 -27 ========================================== - Hits 16508 16481 -27 Misses 1689 1689 Partials 778 778 ``` | [Flag](https://app.codecov.io/gh/CQCL/hugr/pull/1219/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL) | Coverage Δ | | |---|---|---| | [rust](https://app.codecov.io/gh/CQCL/hugr/pull/1219/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL) | `86.46% <78.37%> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

acl-cqc commented 1 week ago

Hmmm, I'm starting to think that having a new type BuilderFunctionTypeSpec (call it FTSpec or ContainerSpec or something maybe) with factory methods new_endo(impl Into<TypeRow>), new(impl Into<TypeRow>, impl Into<TypeRow>), new_fixed_exts(impl Into<TypeRow>, impl Into<TypeRow>, impl Into<ExtensionSet>), maybe alsonew_endo_fixed_exts might be the best way to go. (new_fixed_exts could also take a FunctionType but taking the three params will require less typing at use sites).

Roughly, we're gonna want to change the builder interface for DFGs, CFGs, and simple-basic-blocks, also non-simple basic blocks + Conditionals + TailLoops (these have an extra row of common inputs), that's gonna be a lot of new methods if we add _endo and _ft variants of each one...

acl-cqc commented 1 week ago

Ok, redone with a couple of helper methods. I admit/agree that ft1 and ft2 are extremely terse names...but that is part of the point.... ;-). (foo,) where foo is a row/type/etc. would be even shorter, but I've not gone there...(I could definitely be persuaded!)