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

Helper method to compare FunctionTypes without checking extension sets. #1209

Closed
aborgna-q opened this issue Jun 19, 2024 · 8 comments · Fixed by #1458
Closed

Helper method to compare FunctionTypes without checking extension sets. #1209

aborgna-q opened this issue Jun 19, 2024 · 8 comments · Fixed by #1458
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@aborgna-q
Copy link
Collaborator

The PartialEq implementation of FunctionType checks that the extension_reqs of both signatures is the same, in addition to the input/output types.

I find myself writing

if s1.input == other.input && s1.output == other.output { ... }

multiple times, when I don't care about mismatching extensions.

We could add a FunctionType::same_types(&self, other) -> bool method or something similar for these comparisons.

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 16, 2024

I wonder if we should just impl Eq for FunctionType and ignore the delta when the extension_inference feature is off?

@aborgna-q
Copy link
Collaborator Author

aborgna-q commented Jul 16, 2024

It's not only about extension_inference.

In tket2 we have multiple occurrences where we don't care about the extensions used inside a definition but only about the actual types coming in an out.

This is normally the case when validating rewrite operations. E.g.
https://github.com/CQCL/tket2/blob/ed263d52ab666e709787da5820124c57678ebecb/tket2-py/src/passes/chunks.rs#L58-L59

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 16, 2024

Hmmmm, in that case the thing you are comparing against really isn't a(nother) FunctionType, but a (TypeRow, TypeRow) or just two TypeRows, right?

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 16, 2024

So you could add FunctionType::get_io(&self) -> (&TypeRow, &TypeRow), say, and then....whether you wanted equal_io(&self, other: (&TypeRow, &TypeRow)) as well, I dunno

@aborgna-q
Copy link
Collaborator Author

Sure, get_io would work and may come useful in more general cases.

@aborgna-q aborgna-q self-assigned this Jul 17, 2024
@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 17, 2024

For a rewrite, we'd expect the replacement to have (same or fewer) requirements, right? So other.inputs == self.inputs && other.outputs == self.outputs && other.extension_reqs.is_subset(self.extension_reqs) ? That could be a method itself...

But might lead to all sorts of awkward questions about covariance/contravariance - what if the return type is a (higher-order) function (value), are we allowed to reduce the extension requirements of that, etc.... get_io (probably) avoids anyone worrying about that :)

@aborgna-q
Copy link
Collaborator Author

we'd expect the replacement to have (same or fewer) requirements

Not necessarily. The example I wrote is part of stitching chunks of a circuit back after being updated. We can do whatever we want with those chunks as long as the signatures(' rows!) match.

@acl-cqc
Copy link
Contributor

acl-cqc commented Jul 17, 2024

we'd expect the replacement to have (same or fewer) requirements

Not necessarily. The example I wrote is part of stitching chunks of a circuit back after being updated. We can do whatever we want with those chunks as long as the signatures(' rows!) match.

If this is post-extension-inference, and you increase the delta, you'll have to update the parent/ancestors (unless they were already loose-upper-bounds that included the new delta!), which could be non-trivial (e.g. updating a FuncDefn requires recursively increasing delta of Calls). So, be warned....(a separate issue though)

@aborgna-q aborgna-q added the good first issue Good for newcomers label Jul 25, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 21, 2024
Small helper method, should be enough to close #1209.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants