Repository navigation
Conversation
| Eq(gd, Derivative(g, d), subdomain=mid)])() | ||
|
|
||
| assert np.any(fd.data != 0) | ||
| assert np.allclose(fd.data, gd.data) |
There was a problem hiding this comment.
At least with so 4, that shouldn't be the case, g.dx reaches into the zeros of g where f isn't zero so they should have different values.
There was a problem hiding this comment.
But f is defined on the subdomain, so it will be reaching into the zeros for both f and g no?
| # Evaluation point relative to the expression's grid, which may be indexed by a | ||
| # SubDimension of `dim` (e.g., a Function on a SubDomain) | ||
| rel = x0 - expr.indices_ref[dim] | ||
| mid = rel.subs({**{i: 0 for i in rel.free_symbols |
There was a problem hiding this comment.
Going through free_symbols seems quite brutal just to handle subdims. Also not sure why it just pops up now SubDimension work fine.
There was a problem hiding this comment.
Agree that this seems quite brutal. Took half a look at fixing before I got distracted. I was surprised by this one as well, but the test below definitely fails on current main with TypeError: Error while computing finite-difference for expr=f(ix, y): Cannot convert expression to float
There was a problem hiding this comment.
Looking at this more,rel.free_symbols is pretty tiny, so it's less bad than it looks. It seems the issue only appears when you have a staggered evaluation with a function on the grid on the LHS and a function on a subdomain on the RHS
There was a problem hiding this comment.
Digging into this did lead me to a bug - it looks like staggered=x gets dropped on the floor for functions on subdomains (you need to use staggered=ix instead, but this isn't necessarily intuitive). Patching currently so that staggering gets normalised
There was a problem hiding this comment.
I mean this just seems to be that indices_ref returns the subdim instead of the root dim
There was a problem hiding this comment.
Yeah, it does, but seems to me to be what it should intuitively return? However, x0 should probably be normalised to match the dimensions of the expression
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3103 +/- ##
==========================================
- Coverage 84.51% 84.50% -0.02%
==========================================
Files 260 260
Lines 58493 58533 +40
Branches 4962 4963 +1
==========================================
+ Hits 49438 49461 +23
- Misses 8226 8242 +16
- Partials 829 830 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… function on subdomain being dropped
f36b51e to
d331857
Compare
|
|
||
| # Express `x0` in the same Dimension as the indices, or the FD weights retain | ||
| # e.g. `iy - y` and are not reduced to numbers | ||
| x0 = expr.indices_ref[dim] + mid * dim.spacing |
There was a problem hiding this comment.
Modifying x0 like that seems quite dangerous
|
Rebase and gtg |
No description provided.