api: Fix handling of staggering in injection/interpolation#2936
api: Fix handling of staggering in injection/interpolation#2936mloubout wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2936 +/- ##
==========================================
+ Coverage 83.33% 83.39% +0.06%
==========================================
Files 248 248
Lines 51799 51881 +82
Branches 4467 4479 +12
==========================================
+ Hits 43165 43267 +102
+ Misses 7882 7861 -21
- Partials 752 753 +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:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mloubout
force-pushed
the
inject-interp
branch
3 times, most recently
from
bc601dc to
74db52a
Compare
|
|
||
| def _field_shifts(self, field): | ||
| """ | ||
| Per-grid-Dimension half-cell shift induced by ``field``'s staggering |
There was a problem hiding this comment.
ultra-nitpick, fixable in a separate PR -- single `, not two, for homogeneity
There was a problem hiding this comment.
We seem to be wildly inconsistent on this
| try: | ||
| staggered = field.staggered | ||
| except AttributeError: | ||
| return None |
There was a problem hiding this comment.
when can u end up in the AttributeError? add a comment?
| try: | ||
| staggered = field.staggered | ||
| except AttributeError: | ||
| return None |
| except AttributeError: | ||
| return None | ||
| if not staggered or all(s == 0 for s in staggered): | ||
| return None |
| for _, g in groupby(pairs, lambda f: f[0].staggered): | ||
| g = list(g) | ||
| g_fields = [f for f, _ in g] | ||
| g_exprs = [e for _, e in g] |
There was a problem hiding this comment.
I think you can do
g_fields, g_exprs = zip(*g)
instead of these two lines
| def _grid_map(self): | ||
| return {} | ||
|
|
||
| @property |
|
|
||
| @property | ||
| def origin(self): | ||
| return DimensionTuple(*[0]*len(self.dimensions), |
There was a problem hiding this comment.
ultra-nitpick: self.ndim == len(self.dimensions)
There was a problem hiding this comment.
No this is a SparseFunction, self.ndim is the number of grid dimensions, this is the number of sdimension of the SparseFunction
| def _pos_symbols(self): | ||
| return [Symbol(name=f'pos{d}', dtype=np.int32) | ||
| for d in self.grid.dimensions] | ||
| @memoized_meth |
There was a problem hiding this comment.
the fact that this had to be turned into a memoized_meth makes me think that it should better be placed in the interpolator classes instead.
Accepting shifts here doesn't make much sense imho.
There was a problem hiding this comment.
I think I probably agree with this
There was a problem hiding this comment.
This is the symbol for the sparse position. And it's not used only for interpolation, it's used for plain guards such as here that are used for guarding generic sparse expressions based on the position
| @cached_property | ||
| def _position_map(self): | ||
| @memoized_meth | ||
| def _position_map(self, shifts=None): |
There was a problem hiding this comment.
same story, imho this method doesn't belong here anymore
There was a problem hiding this comment.
It does, this is a SparseFunction information which is it's position relative to an origin. The interpolator uses it, it doesn't define it.
| "assert np.isclose(norm(rec), 31.23, atol=0, rtol=1e-3)\n", | ||
| "assert np.isclose(norm(rec2), 3.5482, atol=0, rtol=1e-3)\n", | ||
| "assert np.isclose(norm(rec3), 4.7007, atol=0, rtol=1e-3)" | ||
| "assert np.isclose(norm(rec), 29.538, atol=0, rtol=1e-3)\n", |
There was a problem hiding this comment.
that's quite the change
There was a problem hiding this comment.
This one inject the source into v_x which changes things quite a bit over long propagation (2 seconds here) since the source position is not properly nterpolated into the staggered location of v_x
|
|
||
| @property | ||
| def origin(self): | ||
| return DimensionTuple(*[0]*len(self.dimensions), |
There was a problem hiding this comment.
Ultra nitpick: DimensionTuple(*[0 for _ in self.dimensions]) would be more readable in this context
| def _pos_symbols(self): | ||
| return [Symbol(name=f'pos{d}', dtype=np.int32) | ||
| for d in self.grid.dimensions] | ||
| @memoized_meth |
There was a problem hiding this comment.
I think I probably agree with this
|
|
||
| # Position map and temporaries for it | ||
| pmap = self._position_map | ||
| pmap = self._position_map() |
There was a problem hiding this comment.
Does this never need to know about the shifts then?
There was a problem hiding this comment.
This is a guard for pure sparse operation, there is no staggered origin involved
There was a problem hiding this comment.
How come this file has been so heavily reworked?
There was a problem hiding this comment.
Just reorganized it was a pain to navigate it, it's 99% the same just lifted in classes
| '(x, y)', '(x, z)', '(y, z)', | ||
| '(x, y, z)' | ||
| ]) | ||
| def test_interpolate_staggered(self, stagg): |
There was a problem hiding this comment.
Note: These three tests are the only new ones in this file
3 participants
No description provided.