API: Support harmonic averaging for parameter Function #2456
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2456 +/- ##
==========================================
- Coverage 87.08% 87.06% -0.03%
==========================================
Files 238 238
Lines 45118 45171 +53
Branches 8408 8417 +9
==========================================
+ Hits 39291 39326 +35
- Misses 5094 5112 +18
Partials 733 733 ☔ View full report in Codecov by Sentry. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
devito/types/basic.py
Outdated
|
|
||
| # Averaging mode for off the grid evaluation | ||
| self._avg_mode = kwargs.get('avg_mode', 'arithmetic') | ||
| assert self._avg_mode in ['arithmetic', 'harmonic'], "Accepted avg_mode " \ |
There was a problem hiding this comment.
this shouldn't be an assert, but rather a raise ValueError
devito/types/basic.py
Outdated
| # Apply interpolation from inner most dim | ||
| for d, i in self._grid_map.items(): | ||
| retval = retval.diff(d, 0, fd_order=2, x0={d: i}) | ||
| retval = retval.diff(d, deriv_order=0, fd_order=2, x0={d: i}).evaluate |
There was a problem hiding this comment.
Always a bit wary of these premature evaluations
| # Evaluate. Since we used `self.function` it will be on the grid when evaluate | ||
| # is called again within FD | ||
| return retval.evaluate | ||
| return retval.evaluate.expand() |
There was a problem hiding this comment.
why the need to expand ?
also, not that we're calling .evaluate multiple times here -- above in the for d, i in self._grid_map loop at each iteration, and then again here.
THere's something weird about this
There was a problem hiding this comment.
Without exapnd you get horrible stuff like 05*(0.5*(0.5 ... instead of 0.125.
This is a trivial square/cube averaging that should be a plain expression anything else is just messing up with the compiler for no reason
| import pytest | ||
| try: | ||
| import pytest | ||
| except: |
There was a problem hiding this comment.
nitpicking: All these imports should be except ImportError
There was a problem hiding this comment.
I thought we needed exactly the opposite?..e.g. pass
devito/types/basic.py
Outdated
|
|
||
| # Base function | ||
| retval = self.function | ||
| retval = 1 / self.function if self._avg_mode == 'harmonic' else self.function |
There was a problem hiding this comment.
nitpicking: for homogeneity with the rest of the codebase, it should be
if cond:
<then part>
else:
<else part>
even if it's a trivial code fragment. But again, nitpicking...
| if self._avg_mode == 'harmonic': | ||
| retval = 1 / retval | ||
|
|
||
| # Evaluate. Since we used `self.function` it will be on the grid when evaluate |
There was a problem hiding this comment.
does this comment still apply?
mloubout
force-pushed
the
avg-mode
branch
3 times, most recently
from
e599547 to
1f13559
Compare
Needed for accurate elastic simulations
On top of #2455