compiler: Add pass to abridge SubDimension names where possible #2269

EdCaunt · 2023-11-17T17:31:47Z

Making the PR to run it through CI and get some opinions. Still needs tests and probably the option to pass a keyword argument to Operator along the lines of shorten_subdims=False (lmk if anyone has a snappier name).

codecov · 2023-11-17T17:47:53Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1b5f8c3) 86.82% compared to head (0d00ecd) 86.83%.

Files	Patch %	Lines
devito/types/grid.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2269      +/-   ##
==========================================
+ Coverage   86.82%   86.83%   +0.01%     
==========================================
  Files         229      229              
  Lines       42387    42435      +48     
  Branches     7845     7868      +23     
==========================================
+ Hits        36803    36850      +47     
- Misses       4924     4926       +2     
+ Partials      660      659       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

review-notebook-app · 2023-11-24T15:22:23Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini · 2023-11-29T13:56:10Z

devito/passes/clusters/aliases.py

@@ -762,6 +762,7 @@ def optimize_schedule_rotations(schedule, sregistry):

        try:
            candidate = k[ridx]
+            print(candidate, type(candidate))


FabioLuporini · 2023-11-29T17:13:44Z

devito/tools/abc.py

@@ -135,6 +135,12 @@ def __init__(self, a, b, c=4):
                args += tuple(getattr(self, i[1:]))
            else:
                args += (getattr(self, i),)
+
+        args = list(args)
+        for k in kwargs.copy():


do u actually need to copy here?

I thought you don't want to actually?

If you don't copy, the dict length changes within the loop and it causes a RunTimeError. The copy is only used so it iterates over a consistent dictionary. It still edits the original kwargs

ah ok:

It still edits the original kwargs

I had missed this one

I typically do list(kwargs) but it doesn't really matter I guess

Is this a preference for consistency?

good question... I think list(...) is more broadly used

FabioLuporini · 2023-11-29T17:27:33Z

devito/passes/iet/misc.py

+            umapper.update({i: replace_parent(i, mapper) for i in uinds})
+
+        for it in tree:
+            mdims = [d for d in FindSymbols('dimensions').visit(it)]


aka mdims = FindSymbols('dimensions').visit(it)

Oh yeah, oops

mloubout

some minor comments, missed a bit of discussion

mloubout · 2023-12-01T03:53:24Z

devito/passes/clusters/aliases.py

-                             symbolic_size=n)
-        dsi = ModuloDimension('%si' % ds, cd, cd + ds - iis, n)
+        ii = ModuloDimension('%sii' % d.root, ds, iis, incr=iib)
+        cd = CustomDimension(name='%s%s' % (d.root, d.root), symbolic_min=ii,


d.root.name, try to avoid "%s" % obj and rather use "%" % obj.name in general can be slow sometimes to convert to string

Fair, I was going off what was previously there. Will edit

mloubout · 2023-12-01T03:54:36Z

devito/passes/clusters/aliases.py

+        ii = ModuloDimension('%sii' % d.root, ds, iis, incr=iib)
+        cd = CustomDimension(name='%s%s' % (d.root, d.root), symbolic_min=ii,
+                             symbolic_max=iib, symbolic_size=n)
+        dsi = ModuloDimension('%si' % ds.root, cd, cd + ds - iis, n)


mloubout · 2023-12-01T03:55:55Z

devito/passes/iet/misc.py

+
+
+def abridge_dim_names(iet):
+    def replace_parent(dim, mapper):


not sure really need a local function for that

Probably true, it was originally much bulkier, but I think it can probably be removed

mloubout · 2023-12-01T03:58:36Z

devito/passes/iet/misc.py

+        for it in tree:
+            uinds = set().union(*[i.uindices for i in it])
+            uinds = [i for i in uinds if i.is_Incr]
+            uinds = [i for i in uinds if i.parent in mapper]


one line if i.is_Incr and i.parent in mapper

mloubout · 2023-12-01T04:00:08Z

devito/passes/iet/misc.py

+        for it in tree:
+            mdims = FindSymbols('dimensions').visit(it)
+            mdims = [d for d in mdims if d.is_Modulo]
+            mdims = [d for d in mdims if d.parent in {**mapper, **umapper}]


d.parent in mapper or d.parent in umapper , {**mapper, **umapper} creates a new dictionary everytime

mloubout · 2023-12-01T04:01:04Z

devito/passes/iet/misc.py

+            mdims = FindSymbols('dimensions').visit(it)
+            mdims = [d for d in mdims if d.is_Modulo]
+            mdims = [d for d in mdims if d.parent in {**mapper, **umapper}]
+            mmapper = {d: replace_parent(d, {**mapper, **umapper}) for d in mdims}


either create that combined dictioanry once or call replace_parent separately for each one

mloubout · 2023-12-01T04:03:30Z

devito/passes/iet/misc.py

+            mdims = [d for d in mdims if d.parent in {**mapper, **umapper}]
+            mmapper = {d: replace_parent(d, {**mapper, **umapper}) for d in mdims}
+            subs.update({i: Uxreplace({**mapper, **umapper, **mmapper}).visit(i)
+                         for i in it})


do you actually need three separate dictionaries if they end up merged for usage anyway?

FabioLuporini

nitpicks left

FabioLuporini · 2023-12-04T09:22:56Z

devito/passes/iet/misc.py

+        mapper.update({i: i._rebuild(i.root.name) for i in inds
+                       if i.root not in it.dimensions and names.count(i.root.name) < 2})
+
+    if mapper:


if you have to open up a scope this big, it's preferable to do the opposite:

if not mapper: return iet <de-indented code>

it's also a useful mental exercise to understand what would happen if you didn't have this if in place. Would the code still work and just behave unoptimally by having to go through useless extra processing? or would it break? if the former, all good. If the latter, sometimes it's an indication of a flaw in the implementation. What case is yours ?

It would just do a bunch of unnecessary stuff to generate an empty dict and eventually return an unaltered iet. It's just there to avoid inefficiency. Will reverse the return like you said though

FabioLuporini · 2023-12-04T09:23:53Z

devito/passes/iet/misc.py

+        mapper.update(umapper)
+
+        for it in tree:
+            mdims = FindSymbols('dimensions').visit(it)


lhs name is wrong. It should be

dims = FindSymbols('dimensions').visit(it)

FabioLuporini · 2023-12-04T09:24:20Z

devito/passes/iet/misc.py

+        for it in tree:
+            mdims = FindSymbols('dimensions').visit(it)
+            mdims = [d for d in mdims if d.is_Modulo]
+            mdims = [d for d in mdims if d.parent in mapper]


these last two lines can (should) be a single line

mdims = [d for d in mdims if d.is_Modulo and d.parent in mapper]

FabioLuporini · 2023-12-04T09:26:51Z

tests/test_subdomains.py

+
+
+class TestMultiSubDimension:
+    def test_rebuild(self):


blank line after "class ..." line

mloubout · 2023-12-04T13:42:28Z

devito/passes/iet/misc.py

+    if not mapper:
+        return iet
+
+    umapper = {}


Why do you need umapper and not just update mapper directly

mloubout · 2023-12-04T13:43:09Z

devito/passes/iet/misc.py

+    for it in tree:
+        dims = FindSymbols('dimensions').visit(it)
+        dims = [d for d in dims if d.is_Modulo and d.parent in mapper]
+        mmapper = {d: d._rebuild(parent=mapper[d.parent]) for d in dims}


same, why not just update mapper ?

mloubout · 2023-12-04T13:44:38Z

devito/types/grid.py

+            try:
+                ((lst, _), (rst, _)) = thickness
+            except ValueError:
+                raise ValueError("Invalid thickness specification: %s" % thickness)


Maybe bit more explicit message, like "expects : ... "?

FabioLuporini · 2023-12-05T08:24:30Z

devito/passes/iet/misc.py

+    subs = {}
+    mapper = {}
+    tree = retrieve_iteration_tree(iet)
+    for it in tree:


also, tiny comment -- what is this block supposed to do?

or perhaps instead of "what", "why" ?

It gets all the indices which are SubDimensions or SubDimension-derived appearing in the expression in the innermost loop nests to assemble substitutions for them. It then checks if it can rename these dimensions to the name of their root without introducing a clash within the inner loop.

However, due to loop structures like

for x in range(x_M): for yi in range(yi_M): ... for yj in range(yj_M): ....

you need to create a global mapper rather than building and applying it on a tree-by-tree basis. Since the mapper isn't complete until the end of the loop, you need to finish the loop before moving onto updating the parents of the unbound indices.

Actually, nvm, seems to have been left over from previous iterations. Has now been condensed down

FabioLuporini · 2023-12-05T08:22:43Z

devito/passes/iet/misc.py

+    tree = retrieve_iteration_tree(iet)
+    for it in tree:
+        indexeds = FindSymbols('indexeds').visit(it.inner)
+        inds = set().union(*[pull_dims(i, flag=False) for i in indexeds])


these are dims. Why do you call them inds?

FabioLuporini · 2023-12-05T08:23:32Z

devito/passes/iet/misc.py

+        indexeds = FindSymbols('indexeds').visit(it.inner)
+        inds = set().union(*[pull_dims(i, flag=False) for i in indexeds])
+        inds = [i for i in inds if any([d.is_Sub for d in i._defines])]
+        inds = [i for i in inds if not i.is_SubIterator]


when we're sure they're dimensions, we typically use the letter d for iteration

I admit this is loosely done, though

devito/passes/iet/misc.py

FabioLuporini · 2023-12-05T08:25:04Z

devito/passes/iet/misc.py

+    if not mapper:
+        return iet
+
+    for it in tree:


some comments here would help

FabioLuporini · 2023-12-05T08:25:56Z

devito/passes/iet/misc.py

+def abridge_dim_names(iet):
+    subs = {}
+    mapper = {}
+    tree = retrieve_iteration_tree(iet)


this should be called trees, not tree

it should also be moved at the top of the function since it will be reused by other code blocks later on

so

def ... trees = ... <blank line> subs = {} ...

…e generated C

FabioLuporini

I think this is now GTG

EdCaunt requested review from FabioLuporini and mloubout November 17, 2023 17:31

FabioLuporini reviewed Nov 29, 2023

View reviewed changes

EdCaunt force-pushed the abridged_names branch from 42da86c to 1a01f7a Compare November 29, 2023 15:02

EdCaunt requested a review from FabioLuporini November 29, 2023 15:26

FabioLuporini reviewed Nov 29, 2023

View reviewed changes

mloubout added the compiler label Dec 1, 2023

mloubout reviewed Dec 1, 2023

View reviewed changes

EdCaunt mentioned this pull request Dec 1, 2023

types: Remove increments from SubDimension names #2257

Closed

FabioLuporini reviewed Dec 4, 2023

View reviewed changes

tests/test_subdomains.py

class TestMultiSubDimension:

def test_rebuild(self):

Copy link

Contributor

FabioLuporini Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line after "class ..." line

mloubout reviewed Dec 4, 2023

View reviewed changes

EdCaunt force-pushed the abridged_names branch from c6b62f9 to 10b7e8c Compare December 4, 2023 16:53

mloubout approved these changes Dec 4, 2023

View reviewed changes

FabioLuporini changed the title ~~compiler: Added pass to abridge SubDimension names where possible~~ compiler: Add pass to abridge SubDimension names where possible Dec 5, 2023

FabioLuporini reviewed Dec 5, 2023

View reviewed changes

compiler: Add pass to abridge SubDimension names where possible in th…

0d00ecd

…e generated C

EdCaunt force-pushed the abridged_names branch from 10b7e8c to 0d00ecd Compare December 5, 2023 10:41

FabioLuporini approved these changes Dec 5, 2023

View reviewed changes

mloubout merged commit 1bf0991 into master Dec 5, 2023
31 checks passed

mloubout deleted the abridged_names branch December 5, 2023 13:00

compiler: Add pass to abridge SubDimension names where possible #2269

compiler: Add pass to abridge SubDimension names where possible #2269

Conversation

EdCaunt commented Nov 17, 2023

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

review-notebook-app bot commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2023 •

edited

Loading