compiler: Nested indexification #1789
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1789 +/- ##
===========================================
- Coverage 89.54% 58.48% -31.07%
===========================================
Files 209 204 -5
Lines 34037 33504 -533
Branches 4417 4384 -33
===========================================
- Hits 30480 19594 -10886
- Misses 3062 13296 +10234
- Partials 495 614 +119
Continue to review full report at Codecov.
|
mloubout
force-pushed
the
func-as-indw
branch
from
8532296 to
fec2b3a
Compare
|
|
I definitely agree. Maybe we should add like a test or two with more complex Eq ? |
| u0.data[:, :] = 2 | ||
| u1.data[:, :] = 1 | ||
|
|
||
| eq0 = Eq(u0._subs(x, u1), 2*u0) |
There was a problem hiding this comment.
why not subs instead of _subs?
There was a problem hiding this comment.
Oh it does the same just quicker through our _subs that bypass some sympy check and stuff, just habit can switch it.
devito/ir/equations/algorithms.py
Outdated
| @@ -160,6 +160,10 @@ def lower_exprs(expressions, **kwargs): | |||
| mapper.update(dimension_map) | |||
| # Add the user-supplied substitutions | |||
| mapper.update(subs) | |||
| # Apply mapper to itself for nested indexing | |||
| mapper = {k: uxreplace(v, mapper) if k.is_Function else v | |||
There was a problem hiding this comment.
does this work for doubly (or arbitrarily) nested Functions?
should this not become (somewhat) a recursive call?
going through the whole mapper here feels weird
There was a problem hiding this comment.
should this not become (somewhat) a recursive call?
Yeah I think so this will only catch single recursion
FabioLuporini
changed the title
mloubout
force-pushed
the
func-as-indw
branch
3 times, most recently
from
e8f785b to
bb9b307
Compare
mloubout
changed the title
mloubout
force-pushed
the
func-as-indw
branch
9 times, most recently
from
24f4e0f to
1b47f15
Compare
| @@ -1078,11 +1078,9 @@ def indexify(self, indices=None, lshift=False, subs=None): | |||
| subs = subs or {} | |||
| subs = [{**{d.spacing: 1, -d.spacing: -1}, **subs} for d in self.dimensions] | |||
|
|
|||
| # Add halo shift | |||
| shift = self._size_nodomain.left if lshift else tuple([0]*len(self.dimensions)) | |||
There was a problem hiding this comment.
Nested lower_expr takes care of it through the indexed part of it
There was a problem hiding this comment.
doesn't it apply to subs as well?
There was a problem hiding this comment.
No subs is needed because of sudomain dimension map in lower_expr, because f.indexe don't have a subdomain
FabioLuporini
changed the title
mloubout
force-pushed
the
func-as-indw
branch
2 times, most recently
from
5167ad5 to
362ec77
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mloubout
force-pushed
the
func-as-indw
branch
5 times, most recently
from
44ffa85 to
7197f81
Compare
…n only appear as index
| @@ -30,6 +30,7 @@ | |||
| " import gempy as gp\n", | |||
| "except ModuleNotFoundError:\n", | |||
| " # Need to install these\n", | |||
| " ! pip install aiohttp==3.7.1\n", | |||
There was a problem hiding this comment.
Any chance you checked whether now, we can have working version without installing specific gempy and specific pyvista? @EdCaunt ?
Fxes bug that comes with the test (fails to compile on master since the functuion
u1only appear as an index and is never lowered and indexified)