Adjust class hierarchy for overlapping stuff #429
Conversation
| class BlockwiseOverlapping(Expr): | ||
| """Super class that operates like a blockwise op | ||
|
|
||
| We require information from neighboring partitions, so we can't prune partitions | ||
| before lowering, but the spec is the same as for Blockwise ops, we don't reoder | ||
| things, alignment stays consistent, ... | ||
|
|
||
| """ | ||
|
|
||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
Why does this exist? Is there some shared structure between these two that we need to handle?
My intuition here is just to have
class Blockwise(Expr):
...
class MapOverlap(Expr):
...At least from this PR it doesn't seem like we're using the intermediate classes.
There was a problem hiding this comment.
Not not yet, are_co_aligned can leverage this for example
Every check that only cares about the partitioning layout should fall back to this base class
There was a problem hiding this comment.
My recommendation would be to wait until we have a demonstrable need, and even in that case to consider using (Blockwise, MapOverlap) instead if possible.
There was a problem hiding this comment.
Updated the PR with the use-case
It's a bit similar to how Blockwise and Elemwise relate to each other
| @@ -2454,7 +2489,7 @@ def non_blockwise_ancestors(expr): | |||
| e = stack.pop() | |||
| if isinstance(e, IO): | |||
| yield e | |||
| elif isinstance(e, Blockwise): | |||
| elif isinstance(e, BlockwiseOverlapping): | |||
There was a problem hiding this comment.
If this is the only application, then this seems lighter weight to me
| elif isinstance(e, BlockwiseOverlapping): | |
| elif isinstance(e, (Blockwise, MapOverlap)): |
Not sure what to think of this (looking into cumulative stuff, so this came up again)
MapOverlap behaves like Blockwise stuff if we look at the expression when trying to make decisions about alignment and things like this but Blockwise obviously isn't quite correct, since we need neighbouring partitions
Injecting another layer solves this kind of, but I don't want to end up in inheritance hell...