compiler: Revamp DDA for SubDimensions - #3033
FabioLuporini wants to merge 5 commits into
Conversation
11f46b0 to
3a0ad97
Compare
3a0ad97 to
68f52ce
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3033 +/- ##
==========================================
+ Coverage 83.93% 84.02% +0.09%
==========================================
Files 258 258
Lines 55676 56154 +478
Branches 4772 4797 +25
==========================================
+ Hits 46732 47185 +453
- Misses 8128 8155 +27
+ Partials 816 814 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mloubout
left a comment
mloubout
left a comment
There was a problem hiding this comment.
I'm not completely sure i follow the reasoning. This seems to add soem very strong constraints on what can be done with subdomains.
| ) from None | ||
| sub_dimensions.append(constructor(f'i{k.name}', k, thickness)) | ||
| sdshape.append(thickness) | ||
| else: |
There was a problem hiding this comment.
not sure why this is better than just having it inthe try?
|
|
||
| def _arg_check(self, args, *_args, **kwargs): | ||
| # These modules depend on Dimension, so importing them above would cycle | ||
| from devito.mpi import mpi_raise # noqa: PLC0415 |
There was a problem hiding this comment.
this wouldn't be an issue with mpi_rais in exception.py with the rest of the exception stuff
| # SubDimensions check for at least space_order interior points | ||
| # at *op.apply time*, accounting for runtime overrides. Without | ||
| # an explicit middle, the gap assumption is unchecked | ||
| gap = sympy.Dummy(nonnegative=True) |
There was a problem hiding this comment.
dropped, revamped, simplified
| return | ||
|
|
||
| d = self.root | ||
| if args.grid is not None and args.grid.is_distributed(d): |
There was a problem hiding this comment.
not sure why kwargs would be ignored if the grid is not distributed
There was a problem hiding this comment.
dropped, revamped, simplified
|
|
||
| # Runtime overrides do not change the compiled stencil order | ||
| items = [f.space_order for f in args.op.input if f.is_DiscreteFunction] | ||
| space_order = max(items, default=0) |
There was a problem hiding this comment.
Unless f is a Function on subdomain that uses that specific dimension i don't see why those checks are needed. This also seemse to be brutal as it checks every function including all of the ones that have nothing to do with this subdimension
There was a problem hiding this comment.
dropped, revamped, simplified
|
about
yes, but basically this is all about ensuring that we're not dealing with degenerate SubDomains (e.g. left and right regions overlapping, or an interior region that doesn't have enough points for the plausible stencils, etc) these conditions are necessary because data dependence analysis has to make some assumptions in order to return True/False in several cases |
Preventing this makes things like (acoustic water, elastic earth) very complicated because if you can't overlap you cannot have transition domain. |
This branch has not been deployed
2 participants
fixes #809