types: Remove increments from SubDimension names #2257

EdCaunt · 2023-11-03T17:24:46Z

Requires a compiler pass to resolve clashing dimension names, which in turn requires additions to uxreplace so it can be used on ConditionalDimensions.

…mes during compilation

…nditions

…ionalDimension parents

codecov · 2023-11-03T17:30:08Z

Codecov Report

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

Comparison is base (dd1b154) 86.90% compared to head (6eeb17b) 86.96%.
Report is 74 commits behind head on master.

Files	Patch %	Lines
devito/symbolics/manipulation.py	94.33%	3 Missing ⚠️
devito/ir/equations/algorithms.py	96.55%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2257      +/-   ##
==========================================
+ Coverage   86.90%   86.96%   +0.06%     
==========================================
  Files         229      229              
  Lines       41545    42262     +717     
  Branches     7660     7809     +149     
==========================================
+ Hits        36105    36755     +650     
- Misses       4818     4860      +42     
- Partials      622      647      +25

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

…for reconstruction

FabioLuporini · 2023-11-07T08:37:32Z

devito/ir/equations/algorithms.py

+        dims = set().union(*tuple(set(i.function.dimensions)
+                                  for i in retrieve_indexed(e)))
+        # Dimensions in conditions and ConditionalDimension parents
+        dims = dims.union(*tuple(pull_dims(d.condition, flag=False)


you probably want to unclutter here and move most of this logic into pull_dims

ideally, all you need to do here would be:

dims = pull_dims(e, flag=...) # Sort for groupby dims = sorted(dims, key=lambda x: x.name)

because here you're performing a search which, in theory, is just one, atomic step of separate_dimensions

IIrc, what's required here is more picky than pull_dims (pull dims selects more dimensions than are needed). It just wants dimensions in the indices, conditional parent, and conditional condition

FabioLuporini · 2023-11-07T08:39:50Z

devito/symbolics/manipulation.py

        self.append(cls)
        _uxreplace_handle.register(cls, _uxreplace_handle_reconstructable)

        for kls, callback in (rkwargs_callback_mapper or {}).items():
            _uxreplace_dispatch.register(kls, callback)

-    def dispatchable(self, obj):
-        return isinstance(obj, tuple(self))
+    def dispatchable(self, obj, deep=False):


So, I'm not sure I fully understand...

For ConditionalDimension:

sometimes you want it deep

but some other times, you don't ?

is this correct? in particular, you want it deep only when you calol it inside separate_dimensions?

Inside separate_dimensions, uxreplace wants to recurse into ConditionalDimensions and update all their args, kwargs, etc as necessary. However, elsewhere in the code, in particular here when called from here, this behaviour may not be desired. In the above case, the ConditionalDimension may have already been indexified and shifted during the lowering of the expressions if it contains terms present in the Eq (in the case that ConditionalDimension is always affected by uxreplace), causing shifting to get applied twice. Hence, the need to be able to have some kind of control over the classes touched by uxreplace on a call-by-call basis

FabioLuporini · 2023-11-07T13:03:12Z

devito/types/dimension.py

@@ -200,6 +200,10 @@ def is_const(self):
    def root(self):
        return self

+    @property
+    def is_Root(self):
+        return self == self.root


is self.root

FabioLuporini · 2023-11-07T13:04:54Z

devito/symbolics/manipulation.py

+# _uxreplace_registry.register(ConditionalDimension, deep=True)
+
+# Registry for dimension uxreplace
+_dxreplace_registry = UxreplaceRegistry()


rename as ReplaceRegistry ?

mloubout · 2023-11-07T14:44:55Z

devito/ir/equations/algorithms.py

+    for e in expressions:
+        # Just want dimensions which appear in the expression
+        # Dimensions in indices
+        dims = set().union(*tuple(set(i.function.dimensions)


Un-necessary extra "set" work, just set().union(*[i.function.dimensions ...])

mloubout · 2023-11-07T14:45:10Z

devito/ir/equations/algorithms.py

+        dims = set().union(*tuple(set(i.function.dimensions)
+                                  for i in retrieve_indexed(e)))
+        # Dimensions in conditions and ConditionalDimension parents
+        dims = dims.union(*tuple(pull_dims(d.condition, flag=False)


same, no need for tuple

mloubout · 2023-11-07T14:47:44Z

devito/ir/equations/algorithms.py

+        dims = dims.union(*tuple(pull_dims(d.condition, flag=False)
+                                 for d in dims if d.is_Conditional
+                                 and d.condition is not None),
+                          set(d.parent for d in dims if d.is_Conditional))


the whole thing seems to be a bit intricated retrieve_dimensions(e, deep=True) + e.implicit_dims should give you all dimensions.
Also why are we ignoring the conditionals with a factor?

The issue is that retrieve_dimensions collects too many things, not too few (even with flag=False). It wants to retrieve only indices in the equation, conditional dimension parent, and dimensions in the conditional dimension condition. Good point on the factor, that should probably be added in.

Fabio pointed out that separate_equations probably wants to be after creation of LoweredEQ, so may need to rethink this anyway.

It wants to retrieve only indices in the equation

Why would a dimsion use as a free symbol rather than an index be ignored? It doesn't change the issue it's gonna be the wrong value if not changed. Any time both the conditional and the parent are used together irrespective of how is a clash.

mloubout · 2023-11-07T14:49:07Z

devito/ir/equations/algorithms.py

+
+        # Group dimensions by matching names
+        groups = tuple(tuple(g) for n, g in groupby(dims, key=lambda x: x.name))
+        clashes = tuple(g for g in groups if len(g) > 1)


you only need generator no need for tuple

mloubout · 2023-11-07T14:50:15Z

devito/ir/equations/algorithms.py

+            rdims = tuple(d for d in c if d.is_Root)
+            ddims = tuple(d for d in c if not d.is_Root)
+
+            for d in (ddims + rdims)[:-1]:


I don't think this is safe, a root dimension should never need to have its name change so this should only loop through ddims and raise an error if there is still clashes

mloubout · 2023-11-07T14:51:10Z

devito/ir/equations/algorithms.py

+                    subs[d] = resolutions[d]
+                else:
+                    try:
+                        subs[d] = d._rebuild(d.name+str(count[d.name]))


devito standard is formating '%s%s' % (d.name, count[d.name]) not string concatenation

mloubout · 2023-11-07T14:51:27Z

devito/ir/equations/algorithms.py

+                        subs[d] = d._rebuild(d.name+str(count[d.name]))
+                        count[d.name] += 1
+                    except KeyError:
+                        subs[d] = d._rebuild(d.name+'0')


same, '%s0' % d.name

mloubout · 2023-11-07T14:55:03Z

devito/ir/equations/algorithms.py

+            ddims = tuple(d for d in c if not d.is_Root)
+
+            for d in (ddims + rdims)[:-1]:
+                if d in resolutions.keys():


and resolutions[d] not in clashes

Need to make sure you don't replace a clash by another but that case gets tricky because you can't set resoltuion[d] anymore or it would replace the existing one by a different one, not sure what the best way is for this very corner case.

I could always check that the incremented dimension name isn't in dims and incrementing until it is not?

also no need for .keys()

mloubout · 2023-11-07T14:56:51Z

devito/symbolics/manipulation.py

+                # the object rather than the property, as the latter can
+                # sometimes have default values which do not match the
+                # default for the class
+                v = {i: getattr(expr, "_"+i) for i in expr.__rkwargs__}


'_%s' % i

What if there isn't one and there is only the self.rkwargs one directly .function for any AbstractFunction

Not quite sure I understand?

Some type have attribute such as self.function that are in rkwargs but self._function doesn't exist

…ctors

review-notebook-app · 2023-11-14T15:36:18Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

FabioLuporini · 2023-11-15T05:17:25Z

devito/ir/equations/algorithms.py

+                    subs[d] = resolutions[d]
+                else:
+                    try:
+                        subs[d] = d._rebuild('%s%s' % (d.name, count[d.name]))


use the sregistry instead of count

You can plumb it down the compiler pass from the caller site -- it's one of the kwargs

FabioLuporini · 2023-11-15T05:18:21Z

devito/ir/equations/equation.py

@@ -220,8 +221,11 @@ def writes(self):
    def xreplace(self, rules):
        return LoweredEq(self.lhs.xreplace(rules), self.rhs.xreplace(rules), **self.state)

-    def func(self, *args):
-        return self._rebuild(*args, evaluate=False)
+    def func(self, *args, **kwargs):


since it's a Reconstructable, and in particular a Pickable, I think that the following should suffice:

func = Pickable._rebuild

FabioLuporini · 2023-11-15T05:20:50Z

devito/ir/support/space.py

+
+
+@_uxreplace_dispatch.register(IterationSpace)
+def _(expr, rule, mode='ux'):


expr should be called ispace

but most importantly, see comments on slack

FabioLuporini · 2023-11-15T05:28:00Z

devito/ir/equations/algorithms.py

+            ddims = tuple(d for d in c if not d.is_Root)
+
+            for d in (ddims + rdims)[:-1]:
+                if d in resolutions.keys():


also no need for .keys()

EdCaunt · 2023-12-01T15:54:17Z

Superseded by #2269

FabioLuporini and others added 14 commits October 18, 2023 08:31

WIP

9dc4682

types: Started work on functionality to resolve clashing dimension na…

da6975b

…mes during compilation

types: Tidy up and investigation into failures

d1a73d7

types: Unclashing now works, but is too eager

0426692

types: Consistent failure pattern now

f2322c5

tests: Tightened relational_classes test

15cb6a9

misc: tidy up

bec6aa7

compiler: Widened dimension search to include ConditionalDimension co…

52a9a6e

…nditions

compiler: Limited dimension extraction to indices and included Condit…

99c4d49

…ionalDimension parents

dsl: Modifications to uxreplace

39aa594

dsl: uxreplace now applies to ConditionalDimension conditions

0442dc2

misc: Tidy up

3b2a7d9

tests: Added test for new uxreplace functionality

b0308a1

misc: Tidy up

f86178b

dsl: Tweaked uxreplace

e95a51b

mloubout added the compiler label Nov 3, 2023

EdCaunt added 3 commits November 6, 2023 15:34

dsl: Added deep mode to uxreplace

59dd3ae

dsl: uxreplace now uses underlying attributes rather than properties …

b946c7c

…for reconstruction

mpi: Extended uxreplace deep option to halotouch

0418235

FabioLuporini reviewed Nov 7, 2023

View reviewed changes

dsl: added dxreplace for uxreplace + dimension substitution

2f64dfc

FabioLuporini reviewed Nov 7, 2023

View reviewed changes

mloubout reviewed Nov 7, 2023

View reviewed changes

EdCaunt added 5 commits November 8, 2023 16:26

compiler: Moved dimension unclashing after creation of LoweredEq

6477cd8

dsl: dxreplace now replaces dimensions inside ConditionalDimension fa…

907d1d4

…ctors

misc: flake8

b29d087

types: Restored increments on SubDimension bounds

6e3f274

types: Updated dimension tagging for SubDomainSet

66abbc5

EdCaunt added 2 commits November 14, 2023 14:11

tests: Updated tree structures to match new dimension names

60a5d0d

examples: Updated the generated code printed in the convection example

fc322c1

examples: Updated the SubDomains notebook

6eeb17b

FabioLuporini requested changes Nov 15, 2023

View reviewed changes

EdCaunt closed this Dec 1, 2023

EdCaunt deleted the dim-naming-unclash branch July 9, 2024 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types: Remove increments from SubDimension names #2257

types: Remove increments from SubDimension names #2257

EdCaunt commented Nov 3, 2023

codecov bot commented Nov 3, 2023 •

edited

Loading

FabioLuporini Nov 7, 2023

EdCaunt Nov 7, 2023

FabioLuporini Nov 7, 2023

EdCaunt Nov 7, 2023

FabioLuporini Nov 7, 2023

FabioLuporini Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

EdCaunt Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

mloubout Nov 7, 2023

EdCaunt Nov 7, 2023

FabioLuporini Nov 15, 2023

mloubout Nov 7, 2023

EdCaunt Nov 7, 2023

mloubout Nov 7, 2023

review-notebook-app bot commented Nov 14, 2023

FabioLuporini Nov 15, 2023

FabioLuporini Nov 15, 2023

FabioLuporini Nov 15, 2023

FabioLuporini Nov 15, 2023

FabioLuporini Nov 15, 2023

EdCaunt commented Dec 1, 2023



		@_uxreplace_dispatch.register(IterationSpace)
		def _(expr, rule, mode='ux'):

types: Remove increments from SubDimension names #2257

types: Remove increments from SubDimension names #2257

Conversation

EdCaunt commented Nov 3, 2023

codecov bot commented Nov 3, 2023 • 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

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

review-notebook-app bot commented Nov 14, 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

EdCaunt commented Dec 1, 2023

codecov bot commented Nov 3, 2023 •

edited

Loading