misc: various Dimension internal fixes #2205

mloubout · 2023-09-11T13:51:05Z

Fixes #2204
Fixes #2132
Fixes #2206
Fixes issue with ConditionalDimension range from @ccuetom

codecov · 2023-09-11T15:57:05Z

Codecov Report

Merging #2205 (97f1cc8) into master (5eaad80) will increase coverage by 0.00%.
The diff coverage is 91.85%.

@@           Coverage Diff           @@
##           master    #2205   +/-   ##
=======================================
  Coverage   87.08%   87.08%           
=======================================
  Files         228      228           
  Lines       40611    40700   +89     
  Branches     7429     7454   +25     
=======================================
+ Hits        35367    35445   +78     
- Misses       4644     4652    +8     
- Partials      600      603    +3

Files Changed	Coverage Δ
tests/test_gpu_openacc.py	`5.66% <0.00%> (-0.04%)`	⬇️
tests/test_gpu_openmp.py	`4.23% <ø> (ø)`
tests/test_mpi.py	`99.18% <0.00%> (ø)`
tests/test_operator.py	`97.54% <0.00%> (-0.19%)`	⬇️
devito/core/autotuning.py	`93.50% <50.00%> (-0.41%)`	⬇️
devito/builtins/initializers.py	`91.41% <60.00%> (-1.05%)`	⬇️
devito/tools/data_structures.py	`71.68% <60.00%> (-0.99%)`	⬇️
devito/ir/stree/algorithms.py	`91.55% <100.00%> (+0.16%)`	⬆️
devito/mpi/halo_scheme.py	`93.05% <100.00%> (+0.07%)`	⬆️
devito/operations/interpolators.py	`98.12% <100.00%> (+0.09%)`	⬆️
... and 11 more

FabioLuporini · 2023-09-16T10:30:02Z

devito/core/autotuning.py

@@ -209,8 +209,11 @@ def init_time_bounds(stepper, at_args, args):
    else:
        at_args[dim.max_name] = at_args[dim.min_name] + options['squeezer']
        if dim.size_name in args:
-            # May need to shrink to avoid OOB accesses
-            at_args[dim.max_name] = min(at_args[dim.max_name], args[dim.max_name])
+            if isinstance(args[dim.size_name], range):


if not isinstance(...): # May need to shrink to avoid OOB accesses at_args[dim.max_name] = min(at_args[dim.max_name], args[dim.max_name])

devito/operations/interpolators.py

FabioLuporini · 2023-09-16T10:31:36Z

devito/operations/interpolators.py

@@ -380,5 +394,6 @@ def interpolation_coeffs(self):
    @property
    def _weights(self):
        ddim, cdim = self.interpolation_coeffs.dimensions[1:]
-        return Mul(*[self.interpolation_coeffs.subs({ddim: ri, cdim: rd-rd.symbolic_min})
+        return Mul(*[self.interpolation_coeffs.subs({ddim: ri,


nitpicking
for better readability, can we introduce mapper = {ddim: ....} on the line above?

FabioLuporini · 2023-09-16T10:33:14Z

devito/operator/operator.py

@@ -584,6 +590,9 @@ def _prepare_arguments(self, autotune=None, **kwargs):
            if configuration['mpi']:
                raise ValueError("Multiple Grids found")
        try:
+            # Take biggest grid, i.e discard grids with subdimensions
+            grids = {g for g in grids if not any(d.is_Sub for d in g.dimensions)}


this needs to be better abstracted in my opinion

I wonder whether we could have a SubGrid object (returned via __new__) to capture subsampled grids. And dramatically simplify our lives here

My main though is that this needs to be part of the Function on subdomain overall, this is a temporary fix for arg processing

What happens if we drop this line and modify the one above as

discretizations = {g for g in discretizations if not any(d.is_Sub for d in g.dimensions)}

This should be enough to filter away the Subdomain grids?

Also: can we not at least add a property to Grid such as g.is_subdomain or g.is_domain or I don't know what name would be better. I feel having properties as opposed to peeking at the individual dimensions would be neater

This should be enough to filter away the Subdomain grids?

No this will create issue for example if there is only one "sub-grid" this would end up empty and leads to missing arguments.

But looks like it's a duplicate from above now might be able to remove that one.

devito/tools/utils.py

FabioLuporini · 2023-09-16T10:34:03Z

devito/types/dense.py

@@ -94,7 +94,7 @@ def __init_finalize__(self, *args, function=None, **kwargs):
            # a reference to the user-provided buffer
            self._initializer = None
            if len(initializer) > 0:
-                self.data_with_halo[:] = initializer
+                self.data_with_halo[:] = initializer[:]


interesting, what's the difference here?

No difference, just debug leftover, the issue was calling __init_finalize__ before setting newobj.function above, needed for first touch init. Slightly safer in case of shape differences though for weird (1, n)/(n, 1) shape where someone might give and (n,) vector

just being paranoid here... are we completely sure both the performance and the semantics of this assignment are exactly the same as master, except for that corner case u mention above?

I worry the new one might be slightly slower

[:] is just a view it has no performance impact.

well the view gets created, so it's not really 0 impact.... but yeah, I agree it's gonna be 0.0000001 in practice 😬

…se function

devito/passes/iet/parpragma.py

devito/ir/stree/algorithms.py

FabioLuporini · 2023-09-20T08:29:44Z

devito/ir/stree/algorithms.py

@@ -147,6 +147,12 @@ def preprocess(clusters, options=None, **kwargs):
            found = []
            for c1 in list(queue):
                distributed_aindices = c1.halo_scheme.distributed_aindices
+                h_indices = set().union(*[(d, d.root)


For homogeneity with the rest of the code below, I would rather write:

distributed_aindices = c1.halo_scheme.distributed_aindices loc_indices = c1.halo_scheme.loc_indices all_loc_indices = set().union(*[d._defines for d in ...]) ...

Note: I'm using _defines above

d_defines ?

devito/ir/stree/algorithms.py

FabioLuporini · 2023-09-20T08:38:09Z

devito/operator/operator.py

@@ -584,6 +590,9 @@ def _prepare_arguments(self, autotune=None, **kwargs):
            if configuration['mpi']:
                raise ValueError("Multiple Grids found")
        try:
+            # Take biggest grid, i.e discard grids with subdimensions
+            grids = {g for g in grids if not any(d.is_Sub for d in g.dimensions)}


What happens if we drop this line and modify the one above as

discretizations = {g for g in discretizations if not any(d.is_Sub for d in g.dimensions)}

This should be enough to filter away the Subdomain grids?

Also: can we not at least add a property to Grid such as g.is_subdomain or g.is_domain or I don't know what name would be better. I feel having properties as opposed to peeking at the individual dimensions would be neater

FabioLuporini · 2023-09-20T08:38:56Z

devito/passes/iet/langbase.py

@@ -336,7 +345,8 @@ def _is_offloadable(self, iet):
        functions = FindSymbols().visit(iet)
        buffers = [f for f in functions if f.is_Array and f._mem_mapped]
        hostfuncs = [f for f in functions if not is_on_device(f, self.gpu_fit)]
-        return not (buffers and hostfuncs)
+
+        return not (hostfuncs and buffers)


leftover change?

devito/passes/iet/parpragma.py

FabioLuporini · 2023-09-20T08:53:00Z

devito/passes/iet/parpragma.py

@@ -377,6 +375,12 @@ def _make_partree(self, candidates, nthreads=None):
                                          ncollapsed=ncollapsed, nthreads=nthreads,
                                          **root.args)
            prefix = []
+        elif all(i.is_ParallelRelaxed for i in candidates) and nthreads is not None:


how about just

elif nthreads is not None:

actually, to mirror what we have in the first if, I might prefer:

else: if nthreads is None: nthreads = self.nthreads_nonaffine chunk_size = ... ... else: body = self.HostIteration(schedule='static', ...) prefix = []

FabioLuporini · 2023-09-20T08:57:35Z

devito/types/dense.py

@@ -94,7 +94,7 @@ def __init_finalize__(self, *args, function=None, **kwargs):
            # a reference to the user-provided buffer
            self._initializer = None
            if len(initializer) > 0:
-                self.data_with_halo[:] = initializer
+                self.data_with_halo[:] = initializer[:]


just being paranoid here... are we completely sure both the performance and the semantics of this assignment are exactly the same as master, except for that corner case u mention above?

FabioLuporini · 2023-09-20T08:58:11Z

devito/types/dense.py

@@ -94,7 +94,7 @@ def __init_finalize__(self, *args, function=None, **kwargs):
            # a reference to the user-provided buffer
            self._initializer = None
            if len(initializer) > 0:
-                self.data_with_halo[:] = initializer
+                self.data_with_halo[:] = initializer[:]


I worry the new one might be slightly slower

FabioLuporini · 2023-09-20T09:01:26Z

tests/test_dle.py

@@ -286,7 +284,7 @@ def test_cache_blocking_structure_optrelax_prec_inject():
                                          'openmp': True,
                                          'par-collapse-ncores': 1}))

-    assert_structure(op, ['t', 't,p_s0_blk0,p_s,rsx,rsy'],
+    assert_structure(op, ['t,p_s0_blk0,p_s', 't,p_s0_blk0,p_s,rsx,rsy'],


CHange of parent hierachy in interp with rx... always being conditional

FabioLuporini · 2023-09-20T12:37:14Z

devito/types/dimension.py

@@ -1446,7 +1452,7 @@ def symbolic_max(self):
    def _arg_names(self):
        return (self.min_name, self.max_name, self.name) + self.parent._arg_names

-    def _arg_defaults(self, _min=None, **kwargs):
+    def _arg_defaults(self, _min=None, size=None, **kwargs):


FabioLuporini · 2023-09-20T12:37:21Z

devito/types/dimension.py

@@ -977,7 +983,7 @@ def symbolic_incr(self):
    def bound_symbols(self):
        return set(self.parent.bound_symbols)

-    def _arg_defaults(self, **kwargs):
+    def _arg_defaults(self, alias=None, **kwargs):


useless change

FabioLuporini · 2023-09-20T12:38:01Z

tests/test_dimension.py

@@ -1515,7 +1515,7 @@ def test_issue_1927(self, factor):

        op = Operator(Eq(f, 1))

-        assert op.arguments()['time_M'] == 4*(save-1)  # == min legal endpoint
+        assert op.arguments()['time_M'] == 4*save-1  # == min legal endpoint


I had a comment here which appearly I forgot to fire up...

anyway: was my old comment (min legal endpoint) wrong? or is the new one to be updated?

The comment is correct, the value wasn't

FabioLuporini · 2023-09-20T12:45:47Z

devito/ir/stree/algorithms.py

+                h_indices = set().union(*[(d, d.root)
+                                          for d in c1.halo_scheme.loc_indices])
+
+                # Skip if the Halo echange would end up outside its need iteration space


couple of typos here

FabioLuporini · 2023-09-20T12:51:17Z

devito/operator/operator.py

        args = kwargs['args'] = args.reduce_all()

        # DiscreteFunctions may be created from CartesianDiscretizations, which in
        # turn could be Grids or SubDomains. Both may provide arguments
        discretizations = {getattr(kwargs[p.name], 'grid', None) for p in overrides}
        discretizations.update({getattr(p, 'grid', None) for p in defaults})
        discretizations.discard(None)
+        # Remove subgrids if multiple grids
+        if len(discretizations) > 1:


We need to coin a term for any CartesianDiscretization that is not the main Grid

Also, we need to distinguish I think between SubDomain and SubGrid. The latter being a coarser representation of the (main) Grid

I don't exactly what names to use, but I feel an increasing need for a set of CartesianDiscretization attributes -- suitably overridden in the various subclasses -- along the lines of

is_root (is_main? is_domain?)
is_subdomain
is_subgrid

etc

and then here just g.is_domain for eaxmple

FabioLuporini · 2023-09-20T12:51:35Z

tests/test_dimension.py

@@ -1515,7 +1515,7 @@ def test_issue_1927(self, factor):

        op = Operator(Eq(f, 1))

-        assert op.arguments()['time_M'] == 4*(save-1)  # == min legal endpoint
+        assert op.arguments()['time_M'] == 4*save-1  # == min legal endpoint


FabioLuporini

GTG , thanks

mloubout added the bug-py-minor label Sep 11, 2023

mloubout force-pushed the misc-issues branch 2 times, most recently from 6ad85cf to 03c25b7 Compare September 11, 2023 13:52

mloubout changed the title ~~api: always use conditional dimension for interpolation radius dim~~ misc: various Dimension internal fixes Sep 11, 2023

mloubout added the API api (symbolics, types, ...) label Sep 11, 2023

mloubout force-pushed the misc-issues branch 3 times, most recently from d09401b to 1519665 Compare September 11, 2023 15:23

mloubout force-pushed the misc-issues branch 6 times, most recently from 2dd25ba to 8ae1f5f Compare September 15, 2023 13:46

api: always use conditional dimension for interpolation radius dim

76a3c4b

mloubout force-pushed the misc-issues branch 8 times, most recently from 610eeb2 to 7d2abb3 Compare September 16, 2023 02:13

FabioLuporini reviewed Sep 16, 2023

View reviewed changes

devito/operations/interpolators.py Show resolved Hide resolved

FabioLuporini reviewed Sep 16, 2023

View reviewed changes

devito/tools/utils.py Show resolved Hide resolved

FabioLuporini reviewed Sep 16, 2023

View reviewed changes

mloubout force-pushed the misc-issues branch from 7d2abb3 to 6f9beba Compare September 18, 2023 03:12

api: process injected expression dimensions in case it's not the spar…

fa1a9b7

…se function

mloubout force-pushed the misc-issues branch from 6f9beba to fa1a9b7 Compare September 18, 2023 12:13

compiler: remove atomic collapse hack

87d8d0e

mloubout force-pushed the misc-issues branch from 6ed3785 to 87d8d0e Compare September 18, 2023 12:38

mloubout commented Sep 18, 2023

View reviewed changes

devito/passes/iet/parpragma.py Show resolved Hide resolved

mloubout force-pushed the misc-issues branch 4 times, most recently from 227730a to 47f6154 Compare September 18, 2023 20:16

compiler: prevent halo to be moved outside their iteration space

f7ab007

mloubout force-pushed the misc-issues branch 2 times, most recently from 963ec1b to fa1a7ac Compare September 19, 2023 14:46

compiler: improve interpolation parallelism

441de0f

mloubout force-pushed the misc-issues branch from fa1a7ac to 441de0f Compare September 19, 2023 16:04

FabioLuporini reviewed Sep 20, 2023

View reviewed changes

mloubout force-pushed the misc-issues branch from a9263b4 to 6613990 Compare September 20, 2023 12:50

compiler: remove redundant subgrid check and cleanup

ab160dd

mloubout force-pushed the misc-issues branch from 6613990 to ab160dd Compare September 20, 2023 12:58

FabioLuporini reviewed Sep 20, 2023

View reviewed changes

api: make SubDimension's side easy to check

97f1cc8

mloubout force-pushed the misc-issues branch from 94b5c07 to 97f1cc8 Compare September 20, 2023 13:50

mloubout mentioned this pull request Sep 20, 2023

Multiple discretization handling #2209

Open

FabioLuporini approved these changes Sep 20, 2023

View reviewed changes

mloubout merged commit 237a4a8 into master Sep 20, 2023
32 checks passed

mloubout deleted the misc-issues branch September 20, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: various Dimension internal fixes #2205

misc: various Dimension internal fixes #2205

mloubout commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

FabioLuporini Sep 16, 2023

FabioLuporini Sep 16, 2023

FabioLuporini Sep 16, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 20, 2023

mloubout Sep 20, 2023

FabioLuporini Sep 16, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

mloubout Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

mloubout Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

mloubout Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini left a comment

misc: various Dimension internal fixes #2205

misc: various Dimension internal fixes #2205

Conversation

mloubout commented Sep 11, 2023 • edited Loading

codecov bot commented Sep 11, 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

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

mloubout commented Sep 11, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading