Bugfixes from shiftedmetric-update - #1306
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 last of these cases and fall back to converting both v and f to field-aligned if either does not have yup/ydown fields. Also add some comments where mixed cases were removed before.
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 remove uses of global 'mesh' in Vpar_Grad_par_LCtoC()
Was previously a typo that resulted in using non-field-aligned f in VDDY, which is incorrect.
The location of the result will in future need to be set before shifting it from field-aligned coordinates, if this is necessary. So it is safer to set the location to outloc as soon as 'Field3D result' is declared. Also in a couple of places just 'return apply*diff(...)' instead of creating an unneeded intermediate variable 'result'.
| // (even if one of v and f has yup/ydown fields, it doesn't make sense to | ||
| // multiply them with one in field-aligned and one in non-field-aligned | ||
| // coordinates) | ||
| Field3D v_fa = vMesh->toFieldAligned(v); |
There was a problem hiding this comment.
Forgive me if this is a silly suggestion, but here we're transforming both input fields and then transforming back because one or other don't have yup/ydown -- would we rather call calcYupYdown on one or more of the inputs instead? This would avoid having to duplicate code and should avoid the need to transform the result field so I think might be faster (assuming toFieldAligned costs about the same as a calcYup/down)?
| // If *UseUpDown is true, field "*" has distinct yup and ydown fields which | ||
| // will be used to calculate a derivative along the magnetic field | ||
| bool vUseUpDown = (v.hasYupYdown() && ((&v.yup() != &v) || (&v.ydown() != &v))); | ||
| bool fUseUpDown = (f.hasYupYdown() && ((&f.yup() != &f) || (&f.ydown() != &f))); |
There was a problem hiding this comment.
You've not changed this here but can you elaborate on why we need to check the yup/ydown are distinct? As long as yup/ydown exist we can use the branch that doesn't have to transform to field aligned. Is this a sneaky way to allow methods that need two guard cells when using identity as the parallel transform?
There was a problem hiding this comment.
Is this a sneaky way to allow methods that need two guard cells when using identity as the parallel transform?
yes, exactly. Can get rid of these in #1287. I guess we could also get rid of them if/when we implement 2 yup/ydown points for shiftedMetric and allow using two guard cells in the yup/ydown branch (if MYG>1).
3 participants
#1176 got a bit clobbered by recent updates, and was not all bugfixes anyway, so I've pulled out and rebased just the bugfixes here.
(This PR includes the commits from #1302.)