feat: Make ComposablePass.__call__ return a Hugr
#2697
Conversation
| passes: list[ComposablePass] | ||
|
|
||
| def __call__(self, hugr: Hugr): | ||
| def __call__(self, hugr: Hugr, inplace: bool = True) -> Hugr: |
There was a problem hiding this comment.
Make sure to overrite _apply and _apply_inline, in case some implementor calls them directly
CalMacCQ
changed the title
ComposablePass return a HugrComposablePass.__call__ return a Hugr
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2697 +/- ##
==========================================
- Coverage 83.54% 83.48% -0.06%
==========================================
Files 266 266
Lines 51714 51650 -64
Branches 47180 47086 -94
==========================================
- Hits 43202 43121 -81
- Misses 6134 6153 +19
+ Partials 2378 2376 -2
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:
|
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Thanks, Callum - think the interface is ok, I don't have a great solution to the override-at-least-one problem, but a couple of things....
| """Call all of the passes in sequence.""" | ||
| def _apply(self, hugr: Hugr) -> Hugr: | ||
| for comp_pass in self.passes: | ||
| res = comp_pass(hugr, inplace=False) |
There was a problem hiding this comment.
This returns the result of the last pass only! You should probably initialize via res = hugr outside the loop and then
| res = comp_pass(hugr, inplace=False) | |
| res = comp_pass(res, inplace=False) |
I'm surprised mypy doesn't complain about this, as if self.passes is empty then return res is returning an uninitialized variable
| # At least one of the following _apply methods must be ovewritten | ||
| def _apply(self, hugr: Hugr) -> Hugr: | ||
| hugr = deepcopy(hugr) | ||
| self._apply_inplace(hugr) |
There was a problem hiding this comment.
If we were really concerned....you could have an _in_progress:bool on the instance of ComposablePass (not sure but this might mean setattr??), and then check that you didn't get back here with that set (and error if you do)....
I don't see a great solution tho, no
Includes an idea for simplifying the protocol's `_apply`/`_apply_inline` from #2697 by providing a helper function instead (859c811). --------- Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Alan Lawrence <[email protected]>
Implementing the revisions to `ComposablePass` and `ComposedPass` as discussed with Agustin. I still find the `_apply` and `_apply_inplace` rather unintuitive (although now I realise that it will work). Looking at the default implementations in the `ComposablePass` protocol these look circular but as you're always overrididing at least one of the `_apply` or `_apply_inplace` methods whenever you implement the protocol. Would be interested to know if there are suggestions to improve this. --------- Co-authored-by: Agustín Borgna <[email protected]> Co-authored-by: Agustín Borgna <[email protected]>
Includes an idea for simplifying the protocol's `_apply`/`_apply_inline` from #2697 by providing a helper function instead (859c811). --------- Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Callum Macpherson <[email protected]> Co-authored-by: Alan Lawrence <[email protected]>
Implementing the revisions to
ComposablePassandComposedPassas discussed with Agustin.I still find the
_applyand_apply_inplacerather unintuitive (although now I realise that it will work). Looking at the default implementations in theComposablePassprotocol these look circular but as you're always overrididing at least one of the_applyor_apply_inplacemethods whenever you implement the protocol.Would be interested to know if there are suggestions to improve this.