compiler: Fix buffer stride runtime mismatch#2950
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2950 +/- ##
==========================================
- Coverage 82.85% 79.36% -3.50%
==========================================
Files 249 249
Lines 52279 52268 -11
Branches 4503 4499 -4
==========================================
- Hits 43318 41483 -1835
- Misses 8188 9976 +1788
- Partials 773 809 +36
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
force-pushed
the
fix-buffer-stride-runtime-mismatch
branch
2 times, most recently
from
dd74d3c to
87857e3
Compare
| pad_key = f.__padding_dtype__ | ||
| else: | ||
| pad_key = None | ||
| pad_key = f.__padding_dtype__ if d is f.dimensions[-1] else None |
There was a problem hiding this comment.
random thought: do we need a dimensions_padded property rather than just [-1]? thinking about what we recently doing in PRO with all those "special" Arrays.
There was a problem hiding this comment.
At some point yes would be good to have
There was a problem hiding this comment.
imho since we're at it, I'd add it now and simply make it return -1 for now. I could use it in PRO too in a few places
| padding = kwargs.get('padding') | ||
| if padding is None: | ||
| padding = ((0, 0),)*self.ndim | ||
| if self.is_autopaddable: |
There was a problem hiding this comment.
with this fix, can we also drop this:
https://github.com/devitocodes/devito/blob/main/devito/types/misc.py#L270-L274
?
FabioLuporini
left a comment
FabioLuporini
left a comment
There was a problem hiding this comment.
some questions, but it's a great catch, and a great cleanup too
EdCaunt
left a comment
EdCaunt
left a comment
There was a problem hiding this comment.
Approved pending Fabio's comments
mloubout
force-pushed
the
fix-buffer-stride-runtime-mismatch
branch
6 times, most recently
from
e561c21 to
0f670a5
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
FabioLuporini
changed the title
| assert len(buffers) == 1, "Unexpected form of multi-level buffering" | ||
| buffer, = buffers | ||
| xd = buffer.indices[dim] | ||
| # The new buffer is fed by `buffer`, so it inherits its padding |
There was a problem hiding this comment.
"# The new buffer is fed by
buffer"
nitpicking, not entirely clear imho
FabioLuporini
left a comment
FabioLuporini
left a comment
There was a problem hiding this comment.
Left some minor comments. Uncontroversial , looks like a great cleanup and long due
| def __padding_setup__(self, padding=None, **kwargs): | ||
| padding = tuple(padding or ((0, 0),)*self.ndim) | ||
| def __padding_setup__(self, **kwargs): | ||
| # `padding=` is never honored: derived from policy to avoid stride |
There was a problem hiding this comment.
Worth emitting a deprecation warning here if a user does specify padding?
| @@ -325,7 +325,7 @@ class TestMetaData: | |||
|
|
|||
| def test_wo_halo_wo_padding(self): | |||
There was a problem hiding this comment.
Nitpick: is the wo_padding part of this test name now redundant?
mloubout
force-pushed
the
fix-buffer-stride-runtime-mismatch
branch
2 times, most recently
from
745fbb5 to
94e11ea
Compare
mloubout
force-pushed
the
fix-buffer-stride-runtime-mismatch
branch
from
94e11ea to
ecfa240
Compare
3 participants
Make sure mapped array use symbolic padding so that it always matches the runtime mapped function and can safely reuse the Function's strydes.