Use input field/vector location for CELL_DEFAULT in vecops by ZedThree · Pull Request #1293 · boutproject/BOUT-dev

ZedThree · 2018-10-03T10:52:21Z

Some operations used CELL_DEFAULT to mean CELL_CENTRE, others used it to mean use the input field/vector location, others didn't handle it at all. Now all vector operations use it to mean use the input field/vector location.

Now consistently treats CELL_DEFAULT

d7919 · 2018-10-03T12:16:30Z

  TRACE("Grad( Field3D )");

-  if(outloc_x == CELL_DEFAULT)
+  if (outloc_x == CELL_DEFAULT) {


I think this might be dangerous, currently assume that either all components are at the same location or all are staggered in direction of component. Here we're allowing some to be set from input field and others to set from argument.

I realise you're not changing this but thought I should flag!

Yes, I think it's probably weird to consider a vector with components at different locations, I'm not sure when you would want that. So I don't really know what use-case this overload is for.

I think it's for the single location overload taking CELL_VSHIFT to set things in the staggered in each direction case. I think we'd like the three location overload to be hidden if this where possible, or possibly just have the two things merged.

Ah, I see! Yes, maybe we just shouldn't expose this as part of the public API

EDIT: and similarly for the other three location overloads

Yup, maybe with an ASSERT for further safety?

ZedThree · 2018-10-03T15:42:42Z

Sounds like CELL_VSHIFT may be generally a little bit broken, or at least suspicious.

This PR is the minimal set of changes to make vecops not completely broken. This is blocking for v4.2, as elm-pb won't run without it, so I'd like to get this in and try and fix vecops properly in either 4.2.1 or 4.3

d7919 · 2018-10-09T08:54:54Z

    outloc_x = f.getLocation();
-  if(outloc_y == CELL_DEFAULT)
+  }
+  if (outloc_y == CELL_DEFAULT) {


As above, can we just pass this through?

ZedThree · 2018-10-09T12:41:30Z

CELL_DEFAULT means "do the right thing, if unambiguous; otherwise, throw", where "do the right thing" covers two input fields at the same location or exactly one staggered and one cell centred. This should go into the documentation for CELL_DEFAULT

CELL_VSHIFT should be invalid for Mesh::coordinates, and Field??::setLocation check should have preconditions.

We need to make sure "top-level" checks take into account if staggered grids are actually be used

coordinates. Replaces throw with ASSERT

d7919 · 2018-10-09T14:13:37Z

A further issue here -- routines like Grad which return a vector don't set the vector's location (particularly tricky for the three location argument versions). This can also impact later routines which try to get the input vector location as the vector location may no longer be set appropriately.

Ensure location of result set in vecops routines

d7919 · 2018-10-09T15:50:25Z

#1308 (into this branch) attempts to fix some of these issues.

Currently impossible to handle

…ult-more-pass-through Vecops consistent cell default add more checking on location

ZedThree · 2018-10-12T12:43:07Z

I'm happy with this. @johnomotani you're basically in charge of staggering code at this point, do you want to sign off on this?

johnomotani · 2018-10-12T14:15:41Z


 /// Advection of a scalar field \p f by a velocity vector \p v
+///
+/// Both vectors must be at the same location, which cannot be CELL_VSHIFT


This should say 'The vector and the field must...'.

johnomotani · 2018-10-12T14:24:25Z

-  result += FDDZ(metric->J*vcn.z, f, outloc);
+  result = FDDX(metric->J * vcn.x, f, outloc);
+  result += FDDY(metric->J * vcn.y, f, outloc);
+  result += FDDZ(metric->J * vcn.z, f, outloc);


locations of J again.

johnomotani · 2018-10-12T14:24:46Z

+  Field3D result(localmesh);
+  result = FDDX(metric->J * vcn.x, f, outloc, method);
+  result += FDDY(metric->J * vcn.y, f, outloc, method);
+  result += FDDZ(metric->J * vcn.z, f, outloc, method);


Also clarify that Div cannot have outloc == VSHIFT

Not currently supported, as it would involve "cross-staggering" terms

ZedThree · 2018-10-15T10:41:31Z

@johnomotani That should be everything fixed now!

Use input field/vector location for CELL_DEFAULT in vecops

bdd81a1

Now consistently treats CELL_DEFAULT

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

d7919 reviewed Oct 3, 2018

View reviewed changes

Comment thread src/field/vecops.cxx

d7919 reviewed Oct 3, 2018

View reviewed changes

Comment thread src/field/vecops.cxx

d7919 reviewed Oct 9, 2018

View reviewed changes

Comment thread src/field/vecops.cxx

d7919 reviewed Oct 9, 2018

View reviewed changes

Comment thread src/field/vecops.cxx

Ensure CELL_DEFAULT and CELL_VSHIFT are forbidden in calls to

291b9f6

coordinates. Replaces throw with ASSERT

ZedThree mentioned this pull request Oct 9, 2018

Version 4.2 release candidate #1305

Merged

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 added 14 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

Remove duplicated ASSERT

546ca78

Remove setLocation calls that are not required

8c571e9

Adding support for CELL_VSHIFT to Grad(Field2D)

1c4f9b9

Deprecating three arg version of Grad(Field3D)

83d6b82

Ensure Grad_perp only works at location of input field

993968c

Updating treatment of location in Curl. Forbid use of CELL_VSHIFT

93330d0

Attempt to fix asserts in V_dot_grad

0669941

Move depecrated into header

db34fd6

Adding some Doxygen comments to mention allowed locations

a11951c

d7919 mentioned this pull request Oct 11, 2018

Rename Mesh::coordinates -> Mesh::getCoordinates; deprecate old name #1310

Merged

ZedThree added 2 commits October 11, 2018 13:27

Remove output location argument from V_dot_Grad

573b1aa

Currently impossible to handle

Remove support for CELL_VSHIFT in V_dot_Grad

7fd82e9

d7919 mentioned this pull request Oct 12, 2018

Vecops consistent cell default add more checking on location #1308

Merged

Merge pull request #1308 from boutproject/vecops-consistent-cell-defa…

77dd5bb

…ult-more-pass-through Vecops consistent cell default add more checking on location

ZedThree changed the base branch from next to v4.2-rc October 12, 2018 12:42

johnomotani requested changes Oct 12, 2018

View reviewed changes

ZedThree added 4 commits October 12, 2018 15:55

Fix V_dot_Grad documentation

fce8979

Fix locations of Jacobian in Div

c9acb64

Also clarify that Div cannot have outloc == VSHIFT

Deprecate Curl overloads that take a location

afe353c

Not currently supported, as it would involve "cross-staggering" terms

Fix whitespace in vecops header

df49dd1

Remove output location from Curl properly

64e24b7

johnomotani approved these changes Oct 15, 2018

View reviewed changes

ZedThree merged commit c7014c2 into v4.2-rc Oct 15, 2018

ZedThree deleted the vecops-consistent-cell-default branch October 15, 2018 12:37

Uh oh!

Conversation

ZedThree commented Oct 3, 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!

ZedThree Oct 3, 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!

Uh oh!

Uh oh!

ZedThree commented Oct 3, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

d7919 commented Oct 9, 2018

Uh oh!

d7919 commented Oct 9, 2018

Uh oh!

ZedThree commented Oct 12, 2018

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!

Uh oh!

ZedThree commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ZedThree Oct 3, 2018 •

edited

Loading

ZedThree commented Oct 9, 2018 •

edited

Loading