api: Fix staggering setup and tensor rebuilding #2583
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2583 +/- ##
==========================================
+ Coverage 91.97% 92.00% +0.02%
==========================================
Files 244 244
Lines 47826 47915 +89
Branches 4207 4222 +15
==========================================
+ Hits 43989 44082 +93
+ Misses 3164 3159 -5
- Partials 673 674 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # compare staggering | ||
| if self.expr.staggered == func.staggered: | ||
| if self.expr.staggered == func.staggered and \ | ||
| not (self.expr.is_Add or self.expr.is_Mul): |
There was a problem hiding this comment.
what if it were a Pow or a trascendental function or something else ? this check isn't very neat
devito/types/basic.py
Outdated
| # Extract the `indices`, as perhaps they're explicitly provided | ||
| dimensions, indices = cls.__indices_setup__(*args, **kwargs) | ||
| dim_args = cls.__indices_setup__(*args, **kwargs) | ||
| try: |
There was a problem hiding this comment.
why not letting __indices_setup__ do all this tinkering?
There was a problem hiding this comment.
That would require to add a bunch of it in all the Array/TempFunction/.... that don't need to know about staggered.
devito/types/basic.py
Outdated
| # when executing __init_finalize__ | ||
| newobj._name = name | ||
| newobj._dimensions = dimensions | ||
| newobj._staggered = staggered |
There was a problem hiding this comment.
so Arrays now get this attribute too, which is not ideal. We may need a further AbstractFunction subclass instead, or something else
There was a problem hiding this comment.
why did you lift it up here? as in, why not enough in DiscreteFunction?
There was a problem hiding this comment.
Because I need it to be in __new__, lifting it up requires a lot of messy tinkering with the init_finalize. But I will have another look.
devito/types/dense.py
Outdated
| return tuple() | ||
| elif stagg is CELL: | ||
| staggered = (sympy.S.One for d in dimensions) | ||
| elif stagg in [NODE]: |
There was a problem hiding this comment.
elif stagg is NODE ?
devito/types/dense.py
Outdated
| """ | ||
| stagg = kwargs.get('staggered') | ||
| if stagg is None: | ||
| return tuple() |
There was a problem hiding this comment.
nitpicking -- return ()
devito/types/dense.py
Outdated
| if stagg is None: | ||
| return tuple() | ||
| elif stagg is CELL: | ||
| staggered = (sympy.S.One for d in dimensions) |
There was a problem hiding this comment.
could also be [sympy.S.One]*len(dimensions)
devito/types/dense.py
Outdated
| * 0 to non-staggered dimensions; | ||
| * 1 to staggered dimensions. | ||
| """ | ||
| stagg = kwargs.get('staggered') |
There was a problem hiding this comment.
nitpicking: I'd call this value staggered, and the other one (the "lowered" staggered" processed instead, or perhaps just a letter, but I would avoid both stagg and staggered
devito/types/dense.py
Outdated
| @cached_property | ||
| def _fd_priority(self): | ||
| return 2.1 if self.staggered in [NODE, None] else 2.2 | ||
| return 2.1 if not self.staggered or all(s == 0 for s in self.staggered) else 2.2 |
There was a problem hiding this comment.
makes me think staggered should be of type class Staggering(tuple) rather just a tuple
mloubout
force-pushed
the
tens-rebuild
branch
2 times, most recently
from
255db5a to
fb08901
Compare
| # in the matrix, so we ust return the class name | ||
| return self.__class__.__name__ | ||
|
|
||
| def _rebuild(self, *args, **kwargs): |
There was a problem hiding this comment.
Note: This is the only addition
devito/types/dense.py
Outdated
| staggered_indices = mapper.values() | ||
|
|
||
| if not staggered: | ||
| staggered_indices = (d for d in dimensions) |
devito/types/dense.py
Outdated
| if not staggered: | ||
| staggered_indices = (d for d in dimensions) | ||
| else: | ||
| # Staggered indices |
devito/types/dense.py
Outdated
| @property | ||
| def is_Staggered(self): | ||
| return self.staggered is not None | ||
| if not self.staggered: |
There was a problem hiding this comment.
just return self.staggered or bool(...) if you want it a bool
devito/types/basic.py
Outdated
| def _rebuild(self, *args, **kwargs): | ||
| # Plain `func` call (row, col, comps) | ||
| if not kwargs.keys() & self.__rkwargs__: | ||
| assert len(args) == 3 |
There was a problem hiding this comment.
instead of an assert I would use a ValueError, since this could easily be a user or developer programming attempted thing no? not entirely sure honetly, but in theory one can do mytensor._rebuild(a) where a is whatever. And that imho deserves a proper exception
| if staggered in [CELL, NODE]: | ||
| staggered_indices = dimensions | ||
| staggered_indices = tuple(args) | ||
| else: |
There was a problem hiding this comment.
ultra-nitpicking , I'd just go with elif staggered: ... and then else (ie denesting everything)
Fix two issues