compiler: add rudimentary support for multi-cond buffering #2838
Conversation
mloubout
force-pushed
the
multi-cond-buffering
branch
from
c4cd3ac to
cb9731e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2838 +/- ##
==========================================
+ Coverage 78.94% 78.97% +0.02%
==========================================
Files 248 248
Lines 50877 50901 +24
Branches 4394 4395 +1
==========================================
+ Hits 40167 40197 +30
+ Misses 9912 9905 -7
- Partials 798 799 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mloubout
force-pushed
the
multi-cond-buffering
branch
from
cb9731e to
e1a2dfb
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mloubout
force-pushed
the
multi-cond-buffering
branch
from
e1a2dfb to
51a7df1
Compare
| "metadata": {}, | ||
| "outputs": [ | ||
| { | ||
| "name": "stderr", |
There was a problem hiding this comment.
Leftover NUMA domain count autodetection failed, assuming 1 from running notebook on Mac
Reply via ReviewNB
EdCaunt
left a comment
EdCaunt
left a comment
There was a problem hiding this comment.
What's the reasoning behind this change?
| factor = 2 | ||
| ntmod = (nt - 1) * factor + 1 | ||
|
|
||
| ct1 = ConditionalDimension(name="ct1", parent=grid.time_dim, |
There was a problem hiding this comment.
This API is pretty unintuitive imo - it isn't really clear to me what the relation keyword does to the condition/factor without digging into documentation
| exprs[i] = e.func(*e.args, conditionals=conditionals) | ||
|
|
||
| guards = {d: sympy.And(*v, evaluate=False) for d, v in guards.items()} | ||
| # Combination mode is And by default and Or if all conditions are |
There was a problem hiding this comment.
"Combination mode is And by default. If all conditions are Or then Or combination mode is used." may be less ambiguous
There was a problem hiding this comment.
(nitpicking, with backtips ` around mode)
| index into arrays. A typical use case is when arrays are accessed | ||
| indirectly via the ``condition`` expression. | ||
| relation: Or/And, default=And | ||
| How this ConditionalDimension will be combined with other ones. |
There was a problem hiding this comment.
Seems slightly unintuitive to specify this here? Surely it would make more sense to specify how ConditionalDimensions are combined at the point of combination?
Perhaps with an API like cdim.and(other) and cdim.or(other) which returns a new ConditionalDimension which combines the conditions of its constituent ConditionalDimensions?
There was a problem hiding this comment.
Surely it would make more sense to specify how ConditionalDimensions are combined at the point of combination?
You cannot so that's not relvant. A Function a defined dimension. It will be combined with any indices and implicit_dims at lowering. Combinining it by hand won't do anything as it will be combined yet again at lowering with And making it useless
There was a problem hiding this comment.
Ah ok, so this is to do with how it's handled during lowering and the combination is not exposed at the API level? It might be worth adding a note to the notebook explaining when you would want to change relation?
The test/MFE |
| eqs.append(Eq(f.forward, T+1, implicit_dims=ctend)) | ||
|
|
||
| # run operator with buffering | ||
| op = Operator(eqs, opt=('streaming', 'buffering')) |
There was a problem hiding this comment.
you don't need or want "streaming" here
mloubout
force-pushed
the
multi-cond-buffering
branch
from
87a50a3 to
1c123cf
Compare
API up to debate, but need something within these line to allow for the MFE (see test)