Vecops consistent cell default add more checking on location by d7919 · Pull Request #1308 · boutproject/BOUT-dev

d7919 · 2018-10-09T15:49:43Z

Provides bugfixes to ensure the vector result has location set.

Also ensure copy/assignment of vectors updates the location of the result.

Ensure location of result set in vecops routines

ZedThree · 2018-10-10T09:14:48Z

+  CELL_LOC outloc = (outloc_x == outloc_y && outloc_x == outloc_z)
+                        ? result.x.getLocation()
+                        : CELL_VSHIFT;
+  result.setLocation(outloc);


Do we also need to make sure the individual components are now all at the same/consistent location as well? (or result.y at least?)

Nope, result.setLocation sets the location on each of the components.

ZedThree · 2018-10-10T09:28:59Z

+  // The following probably belongs in the FDD? routines rather than here
+  // as we don't directly interact v and f. Note also no equivalent in Field3D
+  // version of this routine.
+  ASSERT1(outloc == f.getLocation());


Could you say why the Field3D version doesn't have this?

I don't know why -- it's just an observation to motivate why we might want to remove/move this!

This is checked by indexFDD*, so we can remove it from here 👍

johnomotani · 2018-10-10T09:52:35Z

  result += FDDZ(metric->J * vcn.z, f, outloc);
  result /= metric->J;

+  result.setLocation(outloc);


This is not necessary, result.location is set already by FDDX. In the Field*D difops this is just skipped, but we could put an ASSERT1(result.getLocation()==outloc) if you want to double-check?

Ah yes the return is a Field not a vector -- I was busy adding these for the routines which return vectors and must have got carried away!

Actually (at least in this branch) FDDX (and indexFDDX) on field2D doesn't set the location as far as I can tell.

@ZedThree, @johnomotani should I fix this missing setLocation here or in a separate PR?

Should note the 2D/3D DDX versions shouldn't need the location setting in vecops either as these are both set in either indexDDX and/or applyXDiff. It's just indexFDDX(Field2D) that doesn't set a location currently.

It's fixed in 1342d87 of #1306. (That's probably why I was thinking it should be fine 😉)

johnomotani · 2018-10-10T09:52:58Z

  result += FDDZ(metric->J * vcn.z, f, outloc, method);
  result /= metric->J;

+  result.setLocation(outloc);


As above, this is not necessary.

d7919 · 2018-10-10T10:19:32Z

Just to note that this requires #1306 to ensure location of Field2D returned by the Div that uses flux methods (e.g. indexFDDX) is set.

johnomotani · 2018-10-10T12:30:52Z


  result.x = DDX(f, outloc);
  result.y = DDY(f, outloc);
  result.z = DDZ(f, outloc);


I think we need to handle outloc=CELL_VSHIFT here. I guess it wasn't needed before because Field2D could only have location CELL_CENTRE so Vector2D couldn't support CELL_VSHIFT. We also need to handle CELL_DEFAULT because we need to use outloc to set the location of result below. I'd do something like:

CELL_LOC outloc_x, outloc_y, outloc_z; if (outloc == CELL_DEFAULT) { outloc = f.getLocation(); } if (outloc == CELL_VSHIFT) { outloc_x = CELL_XLOW; outloc_y = CELL_YLOW; outloc_z = CELL_ZLOW; } else { outloc_x = outloc; outloc_y = outloc; outloc_z = outloc; }

and then pass outloc_* through to DDX/DDY/DDZ below. I'd be inclined to do the same in Vector3D Grad() and get rid of the version with 3 outloc arguments.

Vector2D can indeed have VSHIFT but there is no handling of it anywhere currently.

We could just use result.setLocation(result.x.getLocation()); to set the location at the end in the meantime. Currently VSHIFT isn't supported for this routine (it should throw if you try) -- changing this may be regarded as a bug fix or it may be a feature!

I agree about trying to get rid of the three argument versions.

johnomotani · 2018-10-10T12:40:44Z

+  Coordinates *metric_z = f.getCoordinates(outloc_z);

  result.x = DDX(f, outloc_x) -
             metric_x->g_12 * DDY(f, outloc_x) / SQ(metric_x->J * metric_x->Bxy);


This is going to fail for CELL_VSHIFT (actually in any case unless all the locations are the same) because DDY is not supported for CELL_CENTRE->CELL_XLOW, etc. (can't interpolate the input to a derivative because the input wouldn't have guard cells set, and can't interpolate the output because the output doesn't have guard cells set).

Maybe Grad_perp shouldn't have an outloc argument at all, since it can only produce output at f's location?

That's a pain. Maybe a check with a nice exception message to say that setting the location is currently unsupported?

EDIT: and to put John's message in the documentation for this function

Have just pushed a commit that tries to address this.

Actually, this is more than a pain, it means CELL_VSHIFT is not terribly useful. Unfortunately, numerically, it is a useful staggering option. Sounds like we will need to put some future work into trying to support interpolating to orthogonal directions in order to better support CELL_VSHIFT

johnomotani · 2018-10-10T12:57:15Z

+const Vector2D V_dot_Grad(const Vector2D &v, const Vector2D &a, CELL_LOC outloc) {
  TRACE("V_dot_Grad( Vector2D , Vector2D )");

+  ASSERT1(outloc != CELL_VSHIFT);


agree with this one, we can't support outloc==CELL_VSHIFT.

johnomotani · 2018-10-10T12:59:20Z

+  ASSERT1(outloc != CELL_VSHIFT);
+  if (outloc == CELL_DEFAULT) {
+    ASSERT1(outloc == v.getLocation() && outloc == a.getLocation());
+    outloc = v.getLocation();


Should these lines be the other way around? at the moment the ASSERT will always fail if outloc == CELL_DEFAULT. If the other way round, only need to check outloc==a.getLocation().

Hmm, that does look strange I think it should perhaps be ?

if (outloc == CELL_DEFAULT) { ASSERT1(v.getLocation() == a.getLocation()); outloc = v.getLocation(); }

and we probably need to assert that v.getLocation isn't VSHIFT

(possibly just move the outloc assert after the if block?)

What combination of three locations are supported?

v and a at the same location, and outloc == CELL_DEFAULT -- this should be fine

everything at CELL_CENTRE -- this should be fine

everything at other locations? -- presumably fine?

v and a at CELL_CENTRE, outloc == CELL_?LOW? vice-versa? -- don't know

v at CELL_CENTRE, a staggered (and vice-versa), outloc == CELL_DEFAULT? -- don't know

If just first two, bring the assert above the if. Not sure about other cases yet.

0669941 should address this. Essentially v and a have to be at the same location and outloc has to also refer to this location (or CELL_DEFAULT).

Just to clarify, just the first three combinations above are supported?

There's a fair bit of information about supported locations coming out of the woodwork in this PR -- can some of it go in the doxygen comments?

v and a at the same location, and outloc == CELL_DEFAULT or outloc==v.getLocation().

I've just added some brief doxygen comments.

johnomotani · 2018-10-10T12:59:34Z

+  ASSERT1(outloc != CELL_VSHIFT);
+  if (outloc == CELL_DEFAULT) {
+    ASSERT1(outloc == v.getLocation() && outloc == a.getLocation());
+    outloc = v.getLocation();


other way round again?

johnomotani · 2018-10-10T12:59:40Z

+  ASSERT1(outloc != CELL_VSHIFT);
+  if (outloc == CELL_DEFAULT) {
+    ASSERT1(outloc == v.getLocation() && outloc == a.getLocation());
+    outloc = v.getLocation();


other way round again?

johnomotani · 2018-10-10T12:59:48Z

+  ASSERT1(outloc != CELL_VSHIFT);
+  if (outloc == CELL_DEFAULT) {
+    ASSERT1(outloc == v.getLocation() && outloc == a.getLocation());
+    outloc = v.getLocation();


other way round again?

d7919 · 2018-10-10T14:40:17Z

I've just pushed something that demos adding CELL_VSHIFT support to Grad(Field2D) and deprecates the three arg version of Grad(Field3D).

I'll try to deprecate the other three arg routines, but let me know if you have any comments on these most recent changes.

ZedThree · 2018-10-10T15:08:59Z


-const Vector3D Grad(const Field3D &f, CELL_LOC outloc_x, CELL_LOC outloc_y,
-                    CELL_LOC outloc_z) {
+const Vector3D DEPRECATED(Grad(const Field3D &f, CELL_LOC outloc_x, CELL_LOC outloc_y,


This should go on the declaration in the header rather than the implementation.

Also, the single-arg version needs to go in the header. Note that I think you'll need to remove the default values for the three-arg version and add one for the single-arg.

EDIT: sorry, latter paragraph applies to Grad_perp not this!

Ah, forgot the header.

Should I remove deprecated from the source file and just put it on header?

Also doesn't removing the default values break backwards compatibility?

ZedThree

DEPRECATED on declaration in header; add single-arg Grad_perp to header

Resolved in db34fd6?

d7919 · 2018-10-10T15:53:52Z

Turns out the outloc argument for V_dot_Grad does not appear in the header for vecops.

Does this mean we can remove it from the implementation without breaking anything?

ZedThree · 2018-10-10T16:00:41Z

So it doesn't! It's currently not possible to call V_dot_Grad(vector, vector)! Yes, remove it from the implementation please!

Looks like it came in in 6fd914d, #1200

d7919 · 2018-10-10T16:02:24Z

So it doesn't! It's currently not possible to call V_dot_Grad(vector, vector)! Yes, remove it from the implementation please!

Looks like it came in in 6fd914d, #1200

So I actually just added it to the header -- have to dash now but feel free to remove this as you see fit!

ZedThree · 2018-10-11T10:36:10Z

If I understand correctly, it basically doesn't make sense for V_dot_Grad to take a location, for any of the overloads? In which case, let's remove the location completely

This also highlights that we don't have any tests for V_dot_Grad, as it's not currently possible to use it!

Currently impossible to handle

ZedThree · 2018-10-11T12:41:07Z

Some big changes here. @johnomotani @d7919 are you both happy with the current state? I'm sure we've missed some edge cases, but vecops is lacking tests -- needs to be remedied for 4.3

d7919 · 2018-10-12T07:49:58Z

I'm going to merge this shortly into the branch for #1293 and then we can review that PR in its entirety.

d7919 added 2 commits October 9, 2018 16:00

Add ASSERTs for checking requested location is unambigous/consistent.

791ebc7

Ensure location of result set in vecops routines

Ensure Vector copy and assignment copies the location

5a079d4

d7919 mentioned this pull request Oct 9, 2018

Use input field/vector location for CELL_DEFAULT in vecops #1293

Merged

d7919 requested a review from ZedThree October 10, 2018 07:39

d7919 added 5 commits October 10, 2018 09:04

Expand unit tests to check that vector copy/assignment copies location

2f39306

Handle CELL_DEFAULT in vector setLocation

b3b756e

Add unit tests for vector setLocation

04f0eca

Fix for vector non-staggered setLocation unit tests

96dc8ab

Add staggered flag check in vector setLocation

ba80a08

ZedThree reviewed Oct 10, 2018

View reviewed changes

johnomotani reviewed Oct 10, 2018

View reviewed changes

d7919 added 2 commits October 10, 2018 11:01

Remove duplicated ASSERT

546ca78

Remove setLocation calls that are not required

8c571e9

d7919 requested review from ZedThree and johnomotani October 10, 2018 12:13

johnomotani reviewed Oct 10, 2018

View reviewed changes

d7919 added 2 commits October 10, 2018 15:38

Adding support for CELL_VSHIFT to Grad(Field2D)

1c4f9b9

Deprecating three arg version of Grad(Field3D)

83d6b82

d7919 added 2 commits October 10, 2018 15:47

Ensure Grad_perp only works at location of input field

993968c

Updating treatment of location in Curl. Forbid use of CELL_VSHIFT

93330d0

ZedThree reviewed Oct 10, 2018

View reviewed changes

ZedThree previously requested changes Oct 10, 2018

View reviewed changes

d7919 added 2 commits October 10, 2018 16:11

Attempt to fix asserts in V_dot_grad

0669941

Move depecrated into header

db34fd6

Adding some Doxygen comments to mention allowed locations

a11951c

ZedThree added this to the BOUT-4.2 milestone Oct 11, 2018

Remove output location argument from V_dot_Grad

573b1aa

Currently impossible to handle

d7919 commented Oct 11, 2018

View reviewed changes

Comment thread src/field/vecops.cxx Outdated

d7919 commented Oct 11, 2018

View reviewed changes

Comment thread src/field/vecops.cxx Outdated

Remove support for CELL_VSHIFT in V_dot_Grad

7fd82e9

d7919 merged commit 77dd5bb into vecops-consistent-cell-default Oct 12, 2018

ZedThree deleted the vecops-consistent-cell-default-more-pass-through branch October 18, 2018 08:56

Uh oh!

Conversation

d7919 commented Oct 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d7919 commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZedThree Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d7919 commented Oct 10, 2018

Uh oh!

ZedThree Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ZedThree Oct 10, 2018 •

edited

Loading

ZedThree Oct 10, 2018 •

edited

Loading

d7919 commented Oct 10, 2018 •

edited

Loading