compiler: Drop SubDomainSet earlier during compilation for more graceful lowering #2393

EdCaunt · 2024-06-28T14:25:43Z

No description provided.

codecov · 2024-06-28T14:28:19Z

Codecov Report

Attention: Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.03%. Comparing base (f518eb3) to head (e9393e7).

Files	Patch %	Lines
devito/passes/clusters/implicit.py	89.28%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f518eb3) and HEAD (e9393e7). Click for more details.

HEAD has 11 uploads less than BASE

Flag BASE (f518eb3) HEAD (e9393e7)

16 5

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2393      +/-   ##
==========================================
- Coverage   86.72%   78.03%   -8.69%     
==========================================
  Files         235      235              
  Lines       44521    44442      -79     
  Branches     8242     5877    -2365     
==========================================
- Hits        38611    34682    -3929     
- Misses       5189     8982    +3793     
- Partials      721      778      +57

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

mloubout · 2024-06-28T14:29:29Z

devito/passes/clusters/implicit.py

@@ -94,8 +91,12 @@ def callback(self, clusters, prefix):
                processed.append(c)
                continue

+            msdims = [msdim(i.dim) for i in c.ispace[idx:]]


{msdim(i.dim) for i in c.ispace[idx:]} - {None}

I suppose the mapper is unordered, so can use a set here. Good spot

mloubout · 2024-06-28T14:29:38Z

devito/passes/clusters/implicit.py

@@ -169,8 +170,12 @@ def callback(self, clusters, prefix):
            except IndexError:
                continue

+            msdims = [i.dim for i in ispace]


mloubout · 2024-06-28T14:29:48Z

devito/passes/clusters/implicit.py

            # Get the dynamic thickness mapper for the given MultiSubDomain
-            mapper, dims = lower_msd(d.msd, c)
+            mapper, dims = lower_msd(msdims)
+            # mapper, dims = lower_msd(d.msd, c)


mloubout · 2024-06-28T14:30:21Z

devito/passes/clusters/implicit.py

+    for d in msdims:
+        # Pull out the parent MultiSubDimension if blocked etc
+        msds = [d for d in d._defines if d.is_MultiSub]
+        assert len(msds) == 1  # Sanity check. MultiSubDimensions shouldn't be nested.


Just for safety before the .pop(). Means that it will throw an error rather than failing cryptically if anything does go wrong/is overlooked

mloubout · 2024-06-28T15:27:43Z

devito/types/grid.py

+                    f[j].data[:] = self._local_bounds[idx]
+
+            d._implicit_dimension = i_dim
+            d._functions = f


modifying d isn't great should rebuild it or create a new one

this is within the lazy constructor, so it's kinda acceptable

Was able to shuffle things around, remove the modification, and reduce the number of lines

devito/types/grid.py

FabioLuporini

Left some comments; note that changing the lower_msd interface will also break PRO

FabioLuporini · 2024-07-01T07:58:40Z

devito/types/grid.py

-        # to create the implicit equations. Untangling this is definitely
-        # possible, but not straightforward
-        self.msd = msd
+    __rargs__ = ('name', 'parent')


Given https://github.com/devitocodes/devito/blob/master/devito/types/dimension.py#L494 I don't think you need this line

FabioLuporini · 2024-07-01T08:04:36Z

devito/passes/clusters/implicit.py

-            mapper, dims = lower_msd(d.msd, c)
+            # Get all MultiSubDimensions in the cluster and get the dynamic thickness
+            # mapper for the associated MultiSubDomain
+            mapper, dims = lower_msd({msdim(i.dim) for i in c.ispace[idx:]} - {None})


I'm not sure I understand this change. If you are visiting d, why do you need its msd as well as any other nested msds ?

I don't think this is particularly clean, but besides that, I'm failing to understand why

lower_msd has been completely replaced. Now stands for "lower multisubdimension" and takes all the multisubdimensions present in the cluster, rather than the multisubdomain attached to the outermost dimension (all references to which have been dropped at this point).

FabioLuporini · 2024-07-01T08:04:44Z

devito/passes/clusters/implicit.py

@@ -170,7 +169,7 @@ def callback(self, clusters, prefix):
                continue

            # Get the dynamic thickness mapper for the given MultiSubDomain
-            mapper, dims = lower_msd(d.msd, c)
+            mapper, dims = lower_msd({i.dim for i in ispace})


same story as above

mloubout · 2024-07-01T16:12:12Z

devito/types/grid.py

+            for j in range(2):
+                idx = 2*i + j
+                if isinstance(self._local_bounds[idx], int):
+                    sd_func.data[:, idx] = np.full((self._n_domains,),


why not just sd_func.data[:, idx] = self._local_bounds[idx] ?

Good spot. Also removes a bunch of extra complexity

FabioLuporini · 2024-07-02T13:01:31Z

devito/passes/clusters/implicit.py

@@ -170,7 +172,7 @@ def callback(self, clusters, prefix):
                continue

            # Get the dynamic thickness mapper for the given MultiSubDomain
-            mapper, dims = lower_msd(d.msd, c)
+            mapper, dims = lower_msd({i.dim for i in ispace}, c)


this is just ispace.itdims

can do in another PR

It'll get forgotten if it isn't done here, so will just do it

…lowering

EdCaunt requested review from FabioLuporini and mloubout June 28, 2024 14:25

EdCaunt requested a review from georgebisbas June 28, 2024 15:21

mloubout reviewed Jun 28, 2024

View reviewed changes

mloubout added the API api (symbolics, types, ...) label Jun 28, 2024

EdCaunt requested a review from mloubout June 28, 2024 16:17

mloubout approved these changes Jun 28, 2024

View reviewed changes

georgebisbas reviewed Jun 28, 2024

View reviewed changes

devito/types/grid.py Show resolved Hide resolved

mloubout added the compiler label Jun 28, 2024

FabioLuporini reviewed Jul 1, 2024

View reviewed changes

EdCaunt force-pushed the subdomainset_rework branch from aa59e18 to 27ad936 Compare July 1, 2024 08:49

EdCaunt requested a review from FabioLuporini July 1, 2024 10:18

mloubout reviewed Jul 1, 2024

View reviewed changes

EdCaunt requested review from georgebisbas and mloubout July 2, 2024 12:50

EdCaunt self-assigned this Jul 2, 2024

FabioLuporini approved these changes Jul 2, 2024

View reviewed changes

EdCaunt force-pushed the subdomainset_rework branch from bacb623 to 14f6fb0 Compare July 2, 2024 13:25

EdCaunt added 11 commits July 9, 2024 11:15

dsl: Initial reworking of MultiSubDimension and SubDomainSet

30711ce

compiler: Drop SubDomainSet earlier in compilation for more graceful …

1ac9617

…lowering

misc: Tidy up

0a5cf1a

dsl: Avoid modification of MultiSubDimensions

84a3ff2

misc: Remove unnecessary set operations

84f8e75

dsl: Remove unnecessary rargs

04a1e26

dsl: Reintroduce singledispatch

15c3180

dsl: Encapsulate bounds with single Function

4851498

tests: Added additional check to SubDomainSet with blocking

b922892

compiler: Better lowering of blocked MultiSubDimensions

503b23b

misc: Minor aestheic tweak

e9393e7

EdCaunt force-pushed the subdomainset_rework branch from 14f6fb0 to e9393e7 Compare July 9, 2024 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Drop SubDomainSet earlier during compilation for more graceful lowering #2393

compiler: Drop SubDomainSet earlier during compilation for more graceful lowering #2393

EdCaunt commented Jun 28, 2024

codecov bot commented Jun 28, 2024 •

edited

Loading

mloubout Jun 28, 2024

EdCaunt Jun 28, 2024

mloubout Jun 28, 2024

mloubout Jun 28, 2024

mloubout Jun 28, 2024

EdCaunt Jun 28, 2024

mloubout Jun 28, 2024

FabioLuporini Jun 28, 2024

EdCaunt Jun 28, 2024

FabioLuporini left a comment

FabioLuporini Jul 1, 2024

FabioLuporini Jul 1, 2024

EdCaunt Jul 1, 2024

FabioLuporini Jul 1, 2024

mloubout Jul 1, 2024

EdCaunt Jul 2, 2024

FabioLuporini Jul 2, 2024

FabioLuporini Jul 2, 2024

EdCaunt Jul 2, 2024

compiler: Drop SubDomainSet earlier during compilation for more graceful lowering #2393

Are you sure you want to change the base?

compiler: Drop SubDomainSet earlier during compilation for more graceful lowering #2393

Conversation

EdCaunt commented Jun 28, 2024

codecov bot commented Jun 28, 2024 • edited Loading

Codecov Report

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

codecov bot commented Jun 28, 2024 •

edited

Loading