dsl: Remove counters for SubDomains and MultiSubDomains #2405
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2405 +/- ##
===========================================
- Coverage 86.74% 63.05% -23.70%
===========================================
Files 235 148 -87
Lines 44553 26737 -17816
Branches 8250 5505 -2745
===========================================
- Hits 38647 16858 -21789
- Misses 5187 9028 +3841
- Partials 719 851 +132 ☔ View full report in Codecov by Sentry. |
| continue | ||
|
|
||
| exprs, thickness = make_implicit_exprs(mapper, self.sregistry) | ||
| try: |
There was a problem hiding this comment.
just an if dim in thicknesses should be fine not need for try
|
|
||
| def _lower_exprs(expressions, subs): | ||
| def _lower_exprs(expressions, subs, **kwargs): | ||
| # FIXME: Not sure why you can get this, but not access with index |
| def _lower_exprs(expressions, subs): | ||
| def _lower_exprs(expressions, subs, **kwargs): | ||
| # FIXME: Not sure why you can get this, but not access with index | ||
| sregistry = kwargs.get('sregistry') |
There was a problem hiding this comment.
From below looks like it's mandatory not optional so need a valid default one not None
There was a problem hiding this comment.
the following is OK:
def _lower_exprs(..., sregistry=None, **kwargs):
<use sregistry>
|
|
||
| def _lower_exprs(expressions, subs): | ||
| def _lower_exprs(expressions, subs, **kwargs): | ||
| # FIXME: Not sure why you can get this, but not access with index |
| def _lower_exprs(expressions, subs): | ||
| def _lower_exprs(expressions, subs, **kwargs): | ||
| # FIXME: Not sure why you can get this, but not access with index | ||
| sregistry = kwargs.get('sregistry') |
There was a problem hiding this comment.
the following is OK:
def _lower_exprs(..., sregistry=None, **kwargs):
<use sregistry>
| # Some Relationals may be pure SymPy objects, thus lacking the subdomain | ||
| dimension_map = {} | ||
|
|
||
| if sregistry and dimension_map: |
There was a problem hiding this comment.
sregistry cannot be none except for perhaps some low-level tests, in which case you may just pass a dummy one instead
| rename_thicknesses(dimension_map, sregistry, rebuilt) | ||
| # Rebuild ConditionalDimensions using rebuilt subdimensions | ||
| # The expression is then rebuilt with this ConditionalDimension | ||
| expr = rebuild_cdims(expr, rebuilt) |
There was a problem hiding this comment.
imho rename_thicknesses and rebuild_cdims should all be part of the same dispatch logic; you may use a ranking system (see https://github.com/devitocodes/devito/blob/master/devito/passes/iet/engine.py#L412-L415 for an example) to ensure certain objects are rebuilt before others.
nitpicking: uniquify_symbols sounds like a better name to me
| if d is None: | ||
| processed.append(c) | ||
| # If no MultiSubDomain present in this cluster, then tip should be reset | ||
| tip = None |
There was a problem hiding this comment.
with thicknesses now in place, do we actually still need this tip thing?
There was a problem hiding this comment.
Yeah, it keeps track of whether consecutive expressions are on the same SubDomainSet, and avoids generating additional unnecessary thickness expressions in this case. However, it did not previously reset if there was an expression without a SubDomainSet in the middle, which meant that you could create cases where the thickness expressions were erroneously omitted.
| dimensions = list(dim.functions.dimensions) | ||
| dimensions[0] = idim | ||
|
|
||
| # TODO: Requires a name change for some reason not to break things. Why? |
There was a problem hiding this comment.
there's a lot of TODOs here
|
Superseded by #2431 |
3 participants
Remove all counters used for
SubDomains andSubDomainSets, instead dynamically renamingSubDimensions during compilation using a symbol registry.Also ships some bugfixes and tests for
SubDomainSet.