dsl: Improve Function rebuilding and use of preexisting arrays as Function data #2438
Conversation
| # If dimensions have been replaced, then it is necessary to set `function` | ||
| # to None. It is also necessary to remove halo and padding kwargs so that | ||
| # they are rebuilt with the new dimensions | ||
| if function is not None and function.dimensions != dimensions: |
There was a problem hiding this comment.
I'm sorry but no we cannot allow that it doesn't matter if it's for internals, this need to go through a __new__ not _rebuild
There was a problem hiding this comment.
But _rebuild() is a __new__ with some extra convenience around it?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2438 +/- ##
==========================================
+ Coverage 87.02% 87.04% +0.01%
==========================================
Files 236 237 +1
Lines 44938 44998 +60
Branches 8369 8372 +3
==========================================
+ Hits 39107 39167 +60
Misses 5102 5102
Partials 729 729 ☔ View full report in Codecov by Sentry. |
devito/types/dense.py
Outdated
|
|
||
| # Don't want to reinitialise array if DataReference used as allocator; | ||
| # create a no-op intialiser to avoid overwriting the original array. | ||
| if isinstance(self._allocator, DataReference): |
There was a problem hiding this comment.
can this be made part of the if-elif-else cascade below?
There was a problem hiding this comment.
No, several branches in the cascade use initialiser which means it needs to be set here to avoid code duplication. Keeping it here also results in more readable code.
tests/test_function.py
Outdated
| assert np.all(f.data_with_halo == g.data_with_halo) | ||
| # Check that it was incremented by two | ||
| check += 2 | ||
| assert np.all(f.data == check) |
There was a problem hiding this comment.
Not sure why you need that "check" array you always know analytical result.
set np.arange to some constant and just check np.all(f.data, a + n)
There was a problem hiding this comment.
You can do that, but it's just more code for the same result. The halo isn't updated, so either you need to skip numbers, or just add to the centre of the array. Either way, it's more code whilst making it less immediately understandable
There was a problem hiding this comment.
Also the amount of skipping changes depending on the MPI config, since there will be different halos
devito/types/dense.py
Outdated
|
|
||
| # Don't want to reinitialise array if DataReference used as allocator; | ||
| # create a no-op intialiser to avoid overwriting the original array. | ||
| if isinstance(self._allocator, DataReference): |
No description provided.