Add missing from field aligned - #1061
Conversation
Advective and flux derivatives, which take v and f as inputs, had cases using yup/ydown fields for one of v and f, but not the other. These do not make sense as multiplication of a field in field-aligned coordinates with another in non-field-aligned coordinates is incorrect. Delete these cases and fall back to converting both v and f to field-aligned if either does not have yup/ydown fields.
In several y-derivatives where we have to convert the input to field-aligned coordinates, the result was not transformed back to the original coordinates.
ec64af4 to
05a2266
Compare
| } | ||
|
|
||
| result = mesh->fromFieldAligned(result); | ||
| result = mesh->fromFieldAligned(result_fa); |
There was a problem hiding this comment.
I'm not sure that result_fa is actually set in the above loop. What happened previously was that result was used to store the field aligned result and then we did an "in-place" transform back into non-field aligned : result = mesh->fromFieldAligned(result);
| else if (vUseUpDown) { | ||
| // Only v has up/down fields | ||
| // f must shift to field aligned coordinates | ||
| Field3D f_fa = mesh->toFieldAligned(f); |
There was a problem hiding this comment.
We're shifting f to be field aligned such that it should make sense to multiply with v, which has field aligned data available (in yup and ydown), so I think this is ok but could be missing something.
There was a problem hiding this comment.
I may well have misunderstood. I thought that to/fromFieldAligned rotate the points in the z-direction by zShift, so the toroidal position of the point (i,j,k) would be different for field-aligned and non-field-aligned fields?
|
|
||
| for(auto &i : result.region(RGN_NOBNDRY)) { | ||
| result[i] = (var_fa[i.yp()] - var_fa[i]) / (metric->dy[i]*sqrt(metric->g_22[i])); | ||
| result_fa[i] = (var_fa[i.yp()] - var_fa[i]) / (metric->dy[i]*sqrt(metric->g_22[i])); |
There was a problem hiding this comment.
I don't think this is required, although it probably doesn't hurt beyond memory usage.
There was a problem hiding this comment.
Agreed it's not required. I thought it would be clearer to keep field-aligned and non-field-aligned fields separate, but am happy to revert back if people think it would be better. You make a good point about the memory usage, I'm inclined now to change it back to result = mesh->fromFieldAligned(result); style.
We can save memory by just using the 'result' variable to store the field aligned result and transform it at the end using 'result = this->fromFieldAligned(result);'
Also remove a 'Field3D result = Field3D(this);' line which seems to have been a typo.
In the unstaggered case, only the value of v is used, so yup/ydown fields for v are not needed.
042426e to
86fb49f
Compare
Was previously a typo that resulted in using non-field-aligned f in VDDY, which is incorrect.
Previously these were incorrectly setting the location of their result to the location of the input variable. This commit changes this to use the specified output location. This probably did not cause bugs as the location was overridden in DDX/DDY/DDZ, etc.
037689c to
4b1706d
Compare
This potentially saves some memory usage.
Codecov Report
@@ Coverage Diff @@
## next #1061 +/- ##
===========================================
+ Coverage 20.16% 39.84% +19.68%
===========================================
Files 162 187 +25
Lines 19814 26322 +6508
===========================================
+ Hits 3995 10488 +6493
- Misses 15819 15834 +15
Continue to review full report at Codecov.
|
|
More up to date version of these fixes is included in #1176 |
3 participants
In several y-derivatives where we have to convert the input to field-aligned coordinates, the result was not transformed back to the original coordinates.
Also, several advective and flux derivatives which take two fields, v and f, as inputs, had cases using yup/ydown fields for one of v and f, but not the other. These do not make sense as multiplication of a field in field-aligned coordinates with another in non-field-aligned coordinates is incorrect. Delete these cases and fall back to converting both v and f to field-aligned if either does not have yup/ydown fields.