Shiftedmetric updates #1176

johnomotani · 2018-07-18T15:32:01Z

A collection of small updates:

fixes to use of transformation to field-aligned coordinates to calculate derivatives for fields that do not have yup/ydown fields
updates to setting of location of fields, to help with using staggered grids
add a method that deletes yup/ydown fields (and will be used to delete field-aligned 'field_fa' member when that is added)
write zShift to the output files to use for post-processing

These updates are in preparation for adding a shifted-metric type class that shifts variables to field-aligned coordinates (which is helpful when also using staggered grids), and for an update to ShiftedMetric to include 2 points up and down the field (if myg>1).

include/bout/paralleltransform.hxx

src/field/field3d.cxx

src/mesh/parallel/shiftedmetric.cxx

src/mesh/index_derivs.cxx

include/bout/paralleltransform.hxx

src/mesh/index_derivs.cxx

johnomotani · 2018-09-27T12:44:11Z

This PR addresses a few things that aren't really related:

bugfixes for shift-to-field-aligned derivatives
setting locations in derivatives
tidying up deletion of yup/ydown fields
fixing invalidation of guard cells in interp_to
enhancements to ShiftedMetric:
- write zShift to output files
- region arguments for to/fromFieldAligned

I think it's ready to merge, but should I split it up?

d7919 · 2018-09-27T12:46:30Z

include/bout/mesh.hxx

@@ -609,12 +609,12 @@ class Mesh {
  typedef BoutReal (*flux_func)(stencil&, stencil &);

  /// Transform a field into field-aligned coordinates
-  const Field3D toFieldAligned(const Field3D &f) {
-    return getParallelTransform().toFieldAligned(f);
+  const Field3D toFieldAligned(const Field3D &f, const REGION region = RGN_NOX) {


Is there a reason for this choice of default region?

I think the reason I did this is that:

we (usually, probably) want to calculate the result in the y-guard cells, to avoid interpolation and needing to apply parallel boundary conditions before taking derivatives or interpolating

we don't want to shift in the corner guard cells, because they contain invalid data and I think I was getting exceptions thrown when including them

we didn't have a region that excludes just the corner guard cells, but includes x- and y-guard cells

since radial derivatives should probably be done in the orthogonal coordinates rather than the field-aligned ones, the x-guard cells shouldn't normally be needed.

d7919 · 2018-09-27T12:47:45Z

include/field3d.hxx

@@ -233,6 +233,11 @@ class Field3D : public Field, public FieldData {
   * Ensure that yup and ydown refer to this field
   */
  void mergeYupYdown();
+
+  /*!
+   * Clear all yup/ydown fields and field_fa


What do you mean by clear (delete, zero out, something else)?

I mean delete (if they exist). It would make more sense to change the name...

d7919 · 2018-09-27T12:50:15Z

src/field/field3d.cxx

@@ -375,6 +378,7 @@ Field3D & Field3D::operator=(const Field3D &rhs) {

  setLocation(rhs.location);

+  clearYupYdown();


Would it be better to copy the yup/ydown (as we do for the data) in this case?

Would it be better to copy the yup/ydown (as we do for the data) in this case?

I don't know. I guess it only makes sense to copy yup/ydown if we also calculate yup/ydown in arithmetic operators. But if we calculate yup/ydown everywhere possible by default, we'd be doing a lot of computations that might not be needed (if we don't take a y-derivative, etc.). It would be nice to give an option for including yup/ydown fields, but I don't see a nice way to do it...

d7919 · 2018-09-27T12:56:24Z

src/mesh/index_derivs.cxx

  Field2D result(this);
  result.allocate(); // Make sure data allocated
+  result.setLocation(loc);


This seems to (potentially) be changing the output location -- it's now certain to be at the requested loc which sounds sensible, but by default I think loc is CELL_DEFAULT and I don't think this makes sense in a call to setLocation

Good point. Fixed in 0e1adbe, and similar issues with indexVDD*(Field2D) in c334b8b.

d7919 · 2018-09-27T12:57:44Z

src/mesh/index_derivs.cxx

@@ -872,6 +866,7 @@ const Field2D Mesh::applyYdiff(const Field2D &var, Mesh::deriv_func func, CELL_L

  Field2D result(this);
  result.allocate(); // Make sure data allocated
+  result.setLocation(diffloc);


This is different to the X functions which now use the passed loc rather than diffloc -- is there a reason for the difference?

It was different because staggering was not implemented for applyYdiff(Field2D). I've implemented it now in 2a35d4d.

d7919 · 2018-09-27T12:58:11Z

src/mesh/index_derivs.cxx

  Field3D result(this);
  result.allocate(); // Make sure data allocated
+  result.setLocation(loc);


This is diffloc in earlier y derivative function.

d7919 · 2018-09-27T12:58:58Z

src/mesh/index_derivs.cxx

-  result = applyYdiff(f, func, diffloc, region);
-
-  result.setLocation(diffloc); // Set the result location
+  Field3D result = applyYdiff(f, func, diffloc, region);


Could just directly return applyYdiff(f, func, diffloc, region); I think.

Yep, updated in ce76aa2.

d7919 · 2018-09-27T13:02:44Z

src/mesh/parallel/shiftedmetric.cxx

-    for(int jy=0;jy<mesh.LocalNy;jy++) {
-      shiftZ(f(jx,jy), mesh.LocalNz, zangle(jx,jy), result(jx,jy));
-    }
+  invalidateGuards(result); // Won't set x-guard cells, so allow checking to throw exception if they are used.


This shouldn't be needed. Creating a field and allocating should give you a field with invalid guards (if check is at the right level). Let me know if this isn't the case and I can look at why/fixing it!

That's right. Sorry, I was trying to be over-cautious. Removed in b4934a5.

d7919 · 2018-09-27T13:04:43Z

src/mesh/parallel/shiftedmetric.cxx

+
+  // We only use methods in ShiftedMetric to get fields for parallel operations
+  // like interp_to or DDY.
+  // Therefore we don't need x-guard cells, so do not set them.


I think we might want to set the x-guard cells in some future situations. Is it easy enough to support this even if the default is to not do it?

I think it's not trivial at the moment, but should be when Region<T> goodness is available #1245, if we define a region with everything but the corner guard cells, which is maybe what RGN_ALL should be?

I wanted to avoid using zShift in the guard cells, because in future at least, zShift on staggered grids might be calculated by interpolating and communicating, so the corner guard cells might not be initialized. At least I think that was the issue I was having before that made valgrind complain, maybe the better solution would just be to make sure the corner guard cells are initialized to something even when checking is turned off?

Remove the intermediate variable result_fa in branch of interp_to that transforms to field-aligned variables. Just store the intermediate result in 'result' instead, to save memory.

Add region arguments (where it is valid to give RGN_NOX or RGN_NOBNDRY) so functions like toFieldAligned/fromFieldAligned can be used both when y-guard cells have been set and when they have not. e.g. when using fromFieldAligned on the results of interpolation or derivatives, y-guard cells cannot be included, but when using toFieldAligned on the inputs to interpolation or derivatives, y-guard cells must be included.

Ensure that no fields in the rhs function/method are using yup/ydown before calling communicate.

Avoids cost of new/delete.

Tests of splitYupYdown() and mergeYupYdown() need to call field.setHasValidYUpDown(true);

Fixing the low-level shiftZ method means that toFieldAligned/fromFieldAligned also do the right thing. Check that input to shiftZ is at same location as the shift angle zShift (i.e. CELL_CENTRE). Set location of result returned from zShift to be the same as the location of the input.

johnomotani · 2018-12-10T17:01:12Z

I've pulled out several of the commits, that were independent of the rest, into separate PRs (#1427, #1432, #1433). What's left is an attempt to delete, or mark as invalid, yup_field and ydown_field members of Field3D when the field is changed by assignment, etc. I think this code now won't go in as-is because of clashes with other changes, e.g. in #1345, but maybe still useful to refer back to if we decide to do something similar...

johnomotani mentioned this pull request Jul 18, 2018

New shifted metric #1177

Closed

ZedThree requested changes Jul 24, 2018

View reviewed changes

johnomotani force-pushed the shiftedmetric-updates branch from 90fd318 to 4790016 Compare September 27, 2018 10:28

johnomotani mentioned this pull request Sep 27, 2018

Add missing from field aligned #1061

Closed

johnomotani force-pushed the shiftedmetric-updates branch from 0add534 to e616269 Compare September 27, 2018 12:34

johnomotani added the bugfix label Sep 27, 2018

d7919 reviewed Sep 27, 2018

View reviewed changes

johnomotani force-pushed the shiftedmetric-updates branch from b4934a5 to 3e7b005 Compare September 28, 2018 11:39

johnomotani mentioned this pull request Oct 9, 2018

Bugfixes from shiftedmetric-update #1306

Merged

johnomotani force-pushed the shiftedmetric-updates branch from 3e7b005 to ab43f72 Compare October 9, 2018 15:07

johnomotani removed the bugfix label Oct 9, 2018

johnomotani mentioned this pull request Oct 11, 2018

No staggering in applyYdiff(Field2D) #1311

Closed

johnomotani added 2 commits October 26, 2018 10:18

Remove result_fa from interp_to

3bda3dc

Remove the intermediate variable result_fa in branch of interp_to that transforms to field-aligned variables. Just store the intermediate result in 'result' instead, to save memory.

Make a method to clean up deleting yup/ydown and field_fa members

d9d0857

johnomotani force-pushed the shiftedmetric-updates branch from ab43f72 to 545aae5 Compare October 26, 2018 13:00

johnomotani force-pushed the shiftedmetric-updates branch from 545aae5 to 79b5157 Compare October 26, 2018 13:05

johnomotani added 2 commits October 26, 2018 15:45

Reset yup/ydown of fields in Solver before running rhs

600cf0b

Ensure that no fields in the rhs function/method are using yup/ydown before calling communicate.

Allow calcYUpDown to be called through Mesh

fb29665

johnomotani mentioned this pull request Oct 29, 2018

Calculate yup/ydown on demand #1342

Closed

Add hasValidYUpDown and set to false instead of calling deleteYupYdown

4a7cbcf

Avoids cost of new/delete.

Update unit tests with setHasValidYUpDown

2d49b15

Tests of splitYupYdown() and mergeYupYdown() need to call field.setHasValidYUpDown(true);

This was referenced Oct 29, 2018

Add multiple parallel slices #1345

Merged

Location of yup/ydown doesn't respect parent location #1276

Closed

johnomotani force-pushed the shiftedmetric-updates branch from 8d9ef31 to d8de292 Compare November 2, 2018 14:33

This was referenced Dec 10, 2018

Remove result_fa from interp_to #1427

Merged

Add region arguments for toFieldAligned/fromFieldAligned #1432

Merged

Location checking in ShiftedMetric::shiftZ() #1433

Merged

johnomotani added the work in progress Not ready for merging label Dec 10, 2018

johnomotani closed this Jan 4, 2019

ZedThree deleted the shiftedmetric-updates branch October 13, 2023 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shiftedmetric updates #1176

Shiftedmetric updates #1176

johnomotani commented Jul 18, 2018

johnomotani commented Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

d7919 Sep 27, 2018

johnomotani Sep 27, 2018 •

edited

Loading

d7919 Sep 27, 2018

johnomotani Sep 27, 2018

johnomotani Sep 27, 2018 •

edited

Loading

johnomotani commented Dec 10, 2018

		@@ -375,6 +378,7 @@ Field3D & Field3D::operator=(const Field3D &rhs) {

		setLocation(rhs.location);

		clearYupYdown();

Shiftedmetric updates #1176

Shiftedmetric updates #1176

Conversation

johnomotani commented Jul 18, 2018

johnomotani commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnomotani Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnomotani Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

johnomotani commented Dec 10, 2018

johnomotani Sep 27, 2018 •

edited

Loading

johnomotani Sep 27, 2018 •

edited

Loading