compiler: Fix issue #2235 #2237

georgebisbas · 2023-10-13T17:21:05Z

No description provided.

codecov · 2023-10-13T17:29:38Z

Codecov Report

Attention: Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.30%. Comparing base (bedb1a9) to head (fc398b6).

Files with missing lines	Patch %	Lines
devito/deprecations.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2237      +/-   ##
==========================================
+ Coverage   87.29%   87.30%   +0.01%     
==========================================
  Files         238      238              
  Lines       45378    45399      +21     
  Branches     4029     4034       +5     
==========================================
+ Hits        39614    39637      +23     
+ Misses       5083     5082       -1     
+ Partials      681      680       -1

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

tests/test_buffering.py

FabioLuporini · 2023-10-16T08:13:15Z

devito/ir/clusters/cluster.py

@@ -343,6 +343,21 @@ def dspace(self):
        intervals = intervals.promote(lambda d: not d.is_Sub)
        intervals = intervals.zero(set(intervals.dimensions) - oobs)

+        # Intersect with intervals from buffered dimensions. Unions of
+        # buffered dimension intervals may result in shrinking time size


"shrinking" feels odd here, since after all time_M will get a larger default

by shrinking here probably you mean that perhaps [0,1] becomes [0,0]

The shrinking is introduced line 298, wouldn't it be cleaner to fix it there with slightly more checks

maybe, not sure, haven't thought about this

part of the exercise, I guess

mloubout · 2023-10-16T11:52:21Z

Also, potentially related, thos tests (and opertor)

devito/tests/test_gpu_common.py

Line 563 in b6f7308

op.apply(time_m=time_m, time_M=nt-2)

Should be valid for time_M=nt-1 but errors due to "OOB"

devito/ir/clusters/cluster.py

georgebisbas · 2023-11-03T16:54:20Z

devito/types/dimension.py

@@ -340,7 +340,7 @@ def _arg_check(self, args, size, interval):
                # Autopadding causes non-integer upper limit
                from devito.symbolics import normalize_args
                upper = interval.upper.subs(normalize_args(args))
-            if args[self.max_name] + upper >= size:
+            if args[self.max_name] + upper > size:


This change seems to be able to unlock this limitation, which is a good thing right?

georgebisbas · 2023-11-03T16:58:06Z

devito/ir/clusters/cluster.py

+        # the higher upper bound available in the involved parts
+        for f, v in parts.items():
+            try:
+                intervals = intervals.ceil(v[f.time_dim])


Any ideas on how to avoid this are welcome

review-notebook-app · 2023-11-08T18:09:16Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

examples/userapi/02_apply.ipynb

FabioLuporini · 2023-11-20T08:31:19Z

devito/ir/support/space.py

@@ -259,6 +259,11 @@ def negate(self):
    def zero(self):
        return Interval(self.dim, 0, 0, self.stamp)

+    def ceil(self, o):


I would turn the API into ceil(self, v) where v is just an integer. It doesn't make much sense to me to "ceil" based on an Interval

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

devito/ir/clusters/cluster.py

+        for f, v in parts.items():
+            for i in v:
+                if i.dim in oobs:
+                    intervals = intervals.ceil(v[i.dim])


as per my comment below, probably this should be i.upper ?

FabioLuporini · 2023-11-20T08:33:10Z

devito/ir/clusters/cluster.py

@@ -362,6 +362,13 @@ def dspace(self):
        intervals = intervals.promote(lambda d: not d.is_Sub)
        intervals = intervals.zero(set(intervals.dimensions) - oobs)

+        # Upper bound of intervals including dimensions classified for


This comment doesn't sound too clear. I think shorter + tiny example might help?

perhaps this is just me nitpicking...

FabioLuporini · 2023-11-20T08:33:34Z

devito/ir/clusters/cluster.py

@@ -362,6 +362,13 @@ def dspace(self):
        intervals = intervals.promote(lambda d: not d.is_Sub)
        intervals = intervals.zero(set(intervals.dimensions) - oobs)

+        # Upper bound of intervals including dimensions classified for
+        # shifting should retain the "oobs" upper bound
+        for f, v in parts.items():


for v in parts.values():

FabioLuporini · 2023-11-20T08:35:00Z

examples/userapi/02_apply.ipynb

@@ -246,14 +250,14 @@
     "name": "stdout",
     "output_type": "stream",
     "text": [
-      "OOB detected due to time_M=2\n"
+      "OOB detected due to time_M=3\n"


I haven't looked at the example code yet, but is this sane?

IOW -- was there an issue with the prior implementation?

tests/test_operator.py

FabioLuporini · 2023-11-24T13:24:53Z

devito/ir/support/space.py

@@ -259,6 +259,9 @@ def negate(self):
    def zero(self):
        return Interval(self.dim, 0, 0, self.stamp)

+    def min_upper(self, v=0):
+        return Interval(self.dim, self.lower, v, self.stamp)


this implementation doesn't really reflect the method name....

it looks like a jump in the class API, given that all other methos are more generic, see zero, flip, lift, etc

maybe use sth like "ceil" ?

To add, should all of these properties be cached? They all build Interval objects, rather than simply returning a private variable

I do not know, @FabioLuporini , @mloubout ?

georgebisbas

@FabioLuporini just a gentle reminder for this

FabioLuporini · 2024-01-19T10:19:53Z

devito/ir/clusters/cluster.py

+                # interval reconstruction
+                if i.dim in oobs and i.dim in f.dimensions:
+                    ii = intervals[i.dim].intersection(v[i.dim])
+                    intervals = intervals.set_upper(i.dim, ii.upper)


question. After this, is it true that intervals[i.dim] == ii ?

There are several tests that have different lower bounds that should not be affected.
We only touch the upper bound here!

Such tests are e.g.

TestConditionalDimension::test_sparse_time_function test_topofusion_w_subdims_conddims test_fission_for_parallelism

georgebisbas · 2024-04-25T11:04:09Z

I tried to rebase this, but it failed on a gradient test. WIll check again

georgebisbas

So I know, it may be bit hard to remember where we have been here, but trying to refresh it.

georgebisbas · 2024-05-09T15:10:42Z

devito/ir/clusters/cluster.py

+                # interval reconstruction
+                if i.dim in oobs and i.dim in f.dimensions:
+                    ii = intervals[i.dim].intersection(v[i.dim])
+                    intervals = intervals.set_upper(i.dim, ii.upper)


There are several tests that have different lower bounds that should not be affected.
We only touch the upper bound here!

Such tests are e.g.

TestConditionalDimension::test_sparse_time_function test_topofusion_w_subdims_conddims test_fission_for_parallelism

georgebisbas · 2024-05-09T15:16:46Z

devito/ir/support/space.py

@@ -259,6 +259,9 @@ def negate(self):
    def zero(self):
        return Interval(self.dim, 0, 0, self.stamp)

+    def min_upper(self, v=0):
+        return Interval(self.dim, self.lower, v, self.stamp)


maybe use sth like "ceil" ?

georgebisbas · 2024-11-15T11:51:44Z

I had honestly forgotten about this, please re-review when available

georgebisbas · 2024-11-15T16:09:33Z

I am having a problem rebasing this due to:

    def test_function_coefficients_xderiv(self):
        p = Dimension('p')

        nstc = 8

        grid = Grid(shape=(51, 51, 51))
        x, y, z = grid.dimensions

        f = Function(name='f', grid=grid, space_order=(2*nstc, 0, 0),
                     coefficients='symbolic')
        g = Function(name='g', grid=grid, space_order=(2*nstc, 0, 0))
        ax = Function(name='DD2x', space_order=0, shape=(2*nstc + 1,),
                      dimensions=(p,))
        ay = Function(name='DD2y', space_order=0, shape=(2*nstc + 1,),
                      dimensions=(p,))
        stencil_coeffs_x_p1 = Coefficient(1, f, x, ax)
        stencil_coeffs_y_p1 = Coefficient(1, f, y, ay)
        stencil_coeffs = Substitutions(stencil_coeffs_x_p1, stencil_coeffs_y_p1)

        eqn = Eq(g, f.dxdy, coefficients=stencil_coeffs)

        op = Operator(eqn)
        op()

where domain should not be relaxed here...

…ime_dims

georgebisbas added the bug-py label Oct 13, 2023

georgebisbas self-assigned this Oct 13, 2023

georgebisbas linked an issue Oct 13, 2023 that may be closed by this pull request

default time_M value in presence of saved TimeFunction is wrong #2235

Open

FabioLuporini reviewed Oct 16, 2023

View reviewed changes

tests/test_buffering.py Outdated Show resolved Hide resolved

FabioLuporini reviewed Oct 16, 2023

View reviewed changes

georgebisbas force-pushed the gb/fix_2235 branch 4 times, most recently from 1a5341f to b1868d3 Compare October 18, 2023 11:59

FabioLuporini requested changes Oct 19, 2023

View reviewed changes

devito/ir/clusters/cluster.py Outdated Show resolved Hide resolved

georgebisbas commented Nov 3, 2023

View reviewed changes

georgebisbas force-pushed the gb/fix_2235 branch from f8900a7 to 1e5101a Compare November 3, 2023 17:18

georgebisbas force-pushed the gb/fix_2235 branch from ea38319 to a0ddb6d Compare November 8, 2023 18:09

georgebisbas force-pushed the gb/fix_2235 branch from a0ddb6d to 01337ef Compare November 16, 2023 12:58

georgebisbas requested review from FabioLuporini and mloubout November 16, 2023 18:20

FabioLuporini requested changes Nov 20, 2023

View reviewed changes

georgebisbas force-pushed the gb/fix_2235 branch 2 times, most recently from df01a7d to 2474a68 Compare November 23, 2023 18:37

FabioLuporini reviewed Nov 24, 2023

View reviewed changes

georgebisbas force-pushed the gb/fix_2235 branch 2 times, most recently from 09daaea to 9fcaba7 Compare November 24, 2023 16:31

georgebisbas requested a review from FabioLuporini November 24, 2023 16:38

georgebisbas force-pushed the gb/fix_2235 branch from 9fcaba7 to 16cf29d Compare January 15, 2024 12:11

georgebisbas commented Jan 18, 2024

View reviewed changes

FabioLuporini reviewed Jan 19, 2024

View reviewed changes

georgebisbas added the compiler label Apr 25, 2024

georgebisbas changed the title ~~tests: Fix issue #2235~~ compiler: Fix issue #2235 Apr 25, 2024

georgebisbas force-pushed the gb/fix_2235 branch from 16cf29d to 3535de8 Compare May 9, 2024 13:16

georgebisbas commented May 9, 2024

View reviewed changes

georgebisbas requested a review from FabioLuporini May 9, 2024 15:18

georgebisbas force-pushed the gb/fix_2235 branch from 3535de8 to f5db549 Compare June 10, 2024 12:17

georgebisbas force-pushed the gb/fix_2235 branch from f5db549 to 53aa1d3 Compare July 12, 2024 16:44

georgebisbas force-pushed the gb/fix_2235 branch from 584757c to 552c480 Compare November 15, 2024 11:43

georgebisbas requested a review from EdCaunt November 15, 2024 11:51

georgebisbas added 12 commits November 20, 2024 17:57

tests: enhance indirection testing

d1b1e69

compiler: Intersect intervals from buffered TimeDimensions

c044537

tests: Edit test_indirection

7c02bbe

compiler: Only relax upper dspace in case of save

8032826

compiler: add mode to IG creation, rebase on 2208

5cb68f7

compiler: simplify solution

0a5c090

compiler: Make DataSpace intervals get the upper limit available of t…

8587533

…ime_dims

tests: Restore redundant, fix tutorial

d3f19af

compiler: Derive solution from oob set of dims

99dd259

compiler: Rework correctness for indirections

3512c28

compiler: Relax intervals with upper from not mapped dimensions

4e1e142

compiler: Fix occurence of NullInterval after rebase

fc398b6

georgebisbas force-pushed the gb/fix_2235 branch from 1398213 to fc398b6 Compare November 20, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Fix issue #2235 #2237

compiler: Fix issue #2235 #2237

georgebisbas commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

FabioLuporini Oct 16, 2023

mloubout Oct 16, 2023

FabioLuporini Oct 17, 2023

mloubout commented Oct 16, 2023

georgebisbas Nov 3, 2023

georgebisbas Nov 3, 2023

review-notebook-app bot commented Nov 8, 2023

FabioLuporini Nov 20, 2023

FabioLuporini Nov 20, 2023

FabioLuporini Nov 20, 2023

FabioLuporini Nov 20, 2023

FabioLuporini Nov 20, 2023

FabioLuporini Nov 24, 2023

georgebisbas Nov 24, 2023

FabioLuporini Nov 24, 2023

georgebisbas May 9, 2024

EdCaunt Nov 18, 2024

georgebisbas Nov 20, 2024

georgebisbas left a comment

FabioLuporini Jan 19, 2024

georgebisbas May 9, 2024

georgebisbas commented Apr 25, 2024

georgebisbas left a comment

georgebisbas May 9, 2024

georgebisbas May 9, 2024

georgebisbas commented Nov 15, 2024

georgebisbas commented Nov 15, 2024

compiler: Fix issue #2235 #2237

Are you sure you want to change the base?

compiler: Fix issue #2235 #2237

Conversation

georgebisbas commented Oct 13, 2023 • edited Loading

codecov bot commented Oct 13, 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

mloubout commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas commented Apr 25, 2024

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas commented Nov 15, 2024

georgebisbas commented Nov 15, 2024

georgebisbas commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading