Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Fix issue #2235 #2237

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

compiler: Fix issue #2235 #2237

wants to merge 13 commits into from

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Oct 13, 2023

No description provided.

@georgebisbas georgebisbas self-assigned this Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 13, 2023

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.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, not sure, haven't thought about this

part of the exercise, I guess

@mloubout
Copy link
Contributor

Also, potentially related, thos tests (and opertor)

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

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

@georgebisbas georgebisbas force-pushed the gb/fix_2235 branch 4 times, most recently from 1a5341f to b1868d3 Compare October 18, 2023 11:59
devito/ir/clusters/cluster.py Outdated Show resolved Hide resolved
@@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas on how to avoid this are welcome

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

examples/userapi/02_apply.ipynb Show resolved Hide resolved
@@ -259,6 +259,11 @@ def negate(self):
def zero(self):
return Interval(self.dim, 0, 0, self.stamp)

def ceil(self, o):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

perhaps this is just me nitpicking...

@@ -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():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for v in parts.values():

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@georgebisbas georgebisbas force-pushed the gb/fix_2235 branch 2 times, most recently from df01a7d to 2474a68 Compare November 23, 2023 18:37
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use sth like "ceil" ?

Copy link
Contributor Author

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FabioLuporini just a gentle reminder for this

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 georgebisbas changed the title tests: Fix issue #2235 compiler: Fix issue #2235 Apr 25, 2024
@georgebisbas
Copy link
Contributor Author

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

Copy link
Contributor Author

@georgebisbas georgebisbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use sth like "ceil" ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default time_M value in presence of saved TimeFunction is wrong
3 participants