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.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.75%. Comparing base (0124871) to head (584757c).
Report is 3 commits behind head on master.

Files	Patch %	Lines
tests/test_operator.py	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2237      +/-   ##
==========================================
+ Coverage   86.74%   86.75%   +0.01%     
==========================================
  Files         235      235              
  Lines       44553    44574      +21     
  Branches     8250     8258       +8     
==========================================
+ Hits        38647    38670      +23     
+ Misses       5187     5186       -1     
+ Partials      719      718       -1

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

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" ?

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" ?

…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 added 12 commits July 12, 2024 17:13

tests: enhance indirection testing

fc49e09

compiler: Intersect intervals from buffered TimeDimensions

8a66b04

tests: Edit test_indirection

f3a91b7

compiler: Only relax upper dspace in case of save

0e04bcf

compiler: add mode to IG creation, rebase on 2208

124a367

compiler: simplify solution

2298fed

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

6f042cc

…ime_dims

tests: Restore redundant

53d895e

examples: Fix tutorial

d9b00a0

compiler: Derive solution from oob set of dims

0575fae

compiler: Rework correctness for indirections

2d61e8b

compiler: Relax intervals with upper from not mapped dimensions

53aa1d3

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

compiler: Fix occurence of NullInterval after rebase

584757c

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

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

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

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 Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading