compiler: Revamp MultiSubDimension lowering #2411
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2411 +/- ##
==========================================
- Coverage 86.75% 86.71% -0.04%
==========================================
Files 235 235
Lines 44610 44652 +42
Branches 8257 8273 +16
==========================================
+ Hits 38702 38721 +19
- Misses 5189 5206 +17
- Partials 719 725 +6 ☔ View full report in Codecov by Sentry. |
devito/types/grid.py
Outdated
| bounds_indices=(2*i, 2*i+1), | ||
| implicit_dimension=i_dim)) | ||
|
|
||
| ltkn = Symbol(name="%s_ltkn%d" % (d.name, counter), dtype=np.int32, |
There was a problem hiding this comment.
NOTE: this is at the moment redundant (see changes to /ir/equations/algorithms above) but it will soon be dropped by a subsequent PR
devito/passes/clusters/implicit.py
Outdated
| pass | ||
| return ispace | ||
| def make_implicit_exprs(mapper): | ||
| return [Eq(list(d._thickness_map)[side], v) for (d, side), v in mapper.items()] |
There was a problem hiding this comment.
No the nicest, would be better to have easy access d.thickness_val(side)
devito/types/grid.py
Outdated
| rtkn = Symbol(name="%s_rtkn%d" % (d.name, counter), dtype=np.int32, | ||
| is_const=True, nonnegative=True) | ||
|
|
||
| left = d.symbolic_min + ltkn |
There was a problem hiding this comment.
Why isn't this done in the Dimension's init?
There was a problem hiding this comment.
Not in AbstractSubDimension.__init__ though, because SubDimension gets custom left/right depending on whether it's a left/right/middle SubDimension
| self.bounds_indices = bounds_indices | ||
| self.implicit_dimension = implicit_dimension | ||
|
|
||
| def __hash__(self): |
There was a problem hiding this comment.
I'm not sure, I will leave it to you to figure it out in your upcoming branch, since conceptually more related to this thing
devito/passes/clusters/implicit.py
Outdated
| @@ -236,8 +221,8 @@ def _lower_msd(dim, cluster): | |||
| @_lower_msd.register(MultiSubDimension) | |||
| def _(dim, cluster): | |||
| i_dim = dim.implicit_dimension | |||
| mapper = {(dim.root, i): dim.functions[i_dim, mM] | |||
| for i, mM in enumerate(dim.bounds_indices)} | |||
| mapper = {t[0]: dim.functions[i_dim, mM] | |||
There was a problem hiding this comment.
@EdCaunt this change broke PRO's sibling PR, will fix
There was a problem hiding this comment.
also, we should avoid magic numbers at all costs, and 0 here is a magic number
There was a problem hiding this comment.
I've got a fix for it already. Will push it
FabioLuporini
force-pushed
the
revamp-msd-lowering
branch
2 times, most recently
from
4e4b416 to
82fa38f
Compare
FabioLuporini
force-pushed
the
revamp-msd-lowering
branch
from
82fa38f to
52d8e09
Compare
devito/passes/clusters/implicit.py
Outdated
| @@ -236,8 +221,8 @@ def _lower_msd(dim, cluster): | |||
| @_lower_msd.register(MultiSubDimension) | |||
| def _(dim, cluster): | |||
| i_dim = dim.implicit_dimension | |||
| mapper = {(dim.root, i): dim.functions[i_dim, mM] | |||
| for i, mM in enumerate(dim.bounds_indices)} | |||
| mapper = {t: dim.functions[i_dim, mM] | |||
There was a problem hiding this comment.
I would avoid for obvious confusion with stepping dim
| bounds_indices=(2*i, 2*i+1), | ||
| implicit_dimension=i_dim)) | ||
|
|
||
| thickness = MultiSubDimension._symbolic_thickness(dname) |
There was a problem hiding this comment.
You can move that in the MultiSubDimension init and just input None
There was a problem hiding this comment.
@EdCaunt can u do it in your upcoming PR
No description provided.