api: Support combination of condition and factor for ConditionalDimension #2413
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2413 +/- ##
=======================================
Coverage 86.72% 86.72%
=======================================
Files 235 235
Lines 44652 44698 +46
Branches 8273 8286 +13
=======================================
+ Hits 38726 38766 +40
- Misses 5195 5207 +12
+ Partials 731 725 -6 ☔ View full report in Codecov by Sentry. |
devito/types/dimension.py
Outdated
| # the index will need to be `self.parent - min(self.condition)` to avoid | ||
| # shifted indexing. E.g if you have `factor=2` and `condition=Ge(time, 10)` | ||
| # then the lowered index needs to be `(time - 10)/ 2` | ||
| ltkn = relational_min(self.condition, self.parent) |
There was a problem hiding this comment.
ltkn stands for "left thickness" -- is it really the name u wanna use here?
(nitpicking)
There was a problem hiding this comment.
Yeah that's intentional that the actual left thickness that is required to be shifted by.
| @@ -927,7 +942,7 @@ def _arg_defaults(self, _min=None, size=None, alias=None): | |||
| # `factor` endpoints are legal, so we return them all. It's then | |||
| # up to the caller to decide which one to pick upon reduction | |||
| dim = alias or self | |||
| if dim._factor is None or size is None: | |||
| if dim.condition is not None or size is None: | |||
There was a problem hiding this comment.
so we don't check _factor anymore here?
There was a problem hiding this comment.
Yeah, basically if there is a condition then you can't safely use the factor to infer arg bounds
devito/types/dimension.py
Outdated
| @@ -892,6 +895,18 @@ def condition(self): | |||
| def indirect(self): | |||
| return self._indirect | |||
|
|
|||
| @property | |||
| def fact_index(self): | |||
There was a problem hiding this comment.
can you not move this whole logic into __init_finalize__ and set index to the proper value from the get go?
devito/ir/equations/equation.py
Outdated
| @@ -188,12 +188,19 @@ def __new__(cls, *args, **kwargs): | |||
| for d in ordering: | |||
| if not d.is_Conditional: | |||
| continue | |||
| if d.condition is None: | |||
| if d._factor is not None: | |||
There was a problem hiding this comment.
d.factor is never None it returns 1 if there is no factor for various reasons
| Infer the minimum valid value for symbol `s` in the expression `expr`. | ||
| For example | ||
| - if `expr` is `s < 10`, then the minimum valid value for `s` is 0 | ||
| - if `expr` is `s >= 10`, then the minimum valid value for `s` is 10 |
There was a problem hiding this comment.
or at least this rang a bell when reading the docstring here
Fixes #2273