Boundary bugfix(es) and exposing `FluxDiv_` interface #558

pgrete · 2021-07-09T17:31:30Z

PR Summary

Also adjust dt calc as the original version resulted in a nan when it was initially set to max

PR Checklist

Code passes cpplint
New features are documented.
Adds a test for any bugs fixed. Adds tests for new features.
Code is formatted
Changes are summarized in CHANGELOG.md
CI has been triggered on Darwin for performance regression tests.
(@lanl.gov employees) Update copyright on changed files

src/driver/driver.cpp

src/bvals/boundary_conditions.cpp

Yurlungur · 2021-07-09T18:02:45Z

src/interface/update.hpp

@@ -33,6 +33,23 @@ namespace parthenon {

 namespace Update {

+// Calculate the flux divergence for a specific component l of a variable v
+KOKKOS_FORCEINLINE_FUNCTION
+Real FluxDiv_(const int l, const int k, const int j, const int i, const int ndim,


We should consider renaming FluxDiv_ to FluxDiv or FluxDivHelper or something if it's publicly exposed.

…ntially repeats work, but is more robust and allows the user to set only the interior in the problem generator

Yurlungur · 2021-07-09T19:14:25Z

@pgrete Changing k-bounds initialized the uninitialized variables in the ghost zones at the initial time. I just pushed to your branch.

pgrete · 2021-07-26T18:42:52Z

What do you think about updating the advection example so that the background has a value of "1" versus "0" currently and then I'd add a PARTHENON_REQUIRE(advected != 0) to the SquareIt function that is enrolled as FillDerived?
This would mean updating the regression data, but I think it's worth adding as is causes almost no overhead an will increase test coverage (namely, that all cells are always initialized).

Yurlungur · 2021-07-26T19:08:59Z

What do you think about updating the advection example so that the background has a value of "1" versus "0" currently and then I'd add a PARTHENON_REQUIRE(advected != 0) to the SquareIt function that is enrolled as FillDerived?
This would mean updating the regression data, but I think it's worth adding as is causes almost no overhead an will increase test coverage (namely, that all cells are always initialized).

I think this is a good idea, so long as the advected field now has whatever form it currently has + 1. ;)

jlippuner

Looks great and great catch!

Since we're making changes to the boundary conditions file anyway, I thought this would be a good opportunity to reduce some code duplication. I've made one generic boundary condition function covering all 12 cases (X1, X2, X3) x (inner, outer) x (outflow, reflection). Hopefully, this would make it easier to avoid such copy&paste errors in the future (if the boundary condition code needs to change). Please let me know what you think.

Yurlungur · 2021-07-28T19:14:45Z

Looks great and great catch!

Since we're making changes to the boundary conditions file anyway, I thought this would be a good opportunity to reduce some code duplication. I've made one generic boundary condition function covering all 12 cases (X1, X2, X3) x (inner, outer) x (outflow, reflection). Hopefully, this would make it easier to avoid such copy&paste errors in the future (if the boundary condition code needs to change). Please let me know what you think.

In principal, I'm in favor of reduced code, but I worry that I can't quite parse whether or not the generic version covers all 12 specialized versions. I'm not sure we have an automated test to catch this. Perhaps we should add one.

jlippuner · 2021-07-28T19:19:32Z

Looks great and great catch!
Since we're making changes to the boundary conditions file anyway, I thought this would be a good opportunity to reduce some code duplication. I've made one generic boundary condition function covering all 12 cases (X1, X2, X3) x (inner, outer) x (outflow, reflection). Hopefully, this would make it easier to avoid such copy&paste errors in the future (if the boundary condition code needs to change). Please let me know what you think.

In principal, I'm in favor of reduced code, but I worry that I can't quite parse whether or not the generic version covers all 12 specialized versions. I'm not sure we have an automated test to catch this. Perhaps we should add one.

We should definitely have a test. However, I think it should be relatively straightforward to check. There's only a few lines that were different between the expanded 12 cases. That being said, I'm happy to move the code duplication reduction to a new PR if it would take too long to review as part of this PR.

Yurlungur · 2021-07-28T19:21:49Z

That being said, I'm happy to move the code duplication reduction to a new PR if it would take too long to review as part of this PR.

That's up to @pgrete I suppose. My preference would be to add an automated test before merging that ensures these are working as intended. Advecting off of each face in 3d would be a. reasonable regression test, I think. We've clearly just been burned by our lack of testing in this area.

pgrete

I like the reduced code (see also comment on readability).
I also think that extending the test coverage is a good idea.
I have an idea to use dc reconstruction with a cfl of 1, so that we an check inflow, outflow, reflect exactly as as there'd be no diffusion.

src/bvals/boundary_conditions.cpp

Co-authored-by: Philipp Grete <gretephi@msu.edu>

jlippuner · 2021-08-05T17:58:13Z

Waiting on test for boundary condition before merging

kurtsansom · 2021-08-24T20:02:03Z

What kind of test are you looking for? simple unit test exercising the templated boundary condition function?

Yurlungur · 2021-08-24T21:00:23Z

What kind of test are you looking for? simple unit test exercising the templated boundary condition function?

Yes exactly. Just to make sure it behaves the same as the older non-templated code. (And/or is correct. ;) )

…-div

…e off

pgrete · 2021-09-12T22:28:45Z

This is now ready for new comments/review.
I now added checks for the new infrastructure (though user boundary conditions are not covered yet, see below).
I tried to be least intrusive to the advection example.
The key changes are:

the FillDerived function now has a conditional that checks if all values are initialized (i.e., making sure the corners are filled, too)
a new regression test, testing periodic, reflecting and outflow conditions enabled
- by migrating individual cells in all directions
- through an optional v(elocity) vector (so that reflecting boundaries are covered)
- a new flux branch in the flux functions that translates cell data for cfl=1.0 (this works as the content of individual cells don't cross)file:///home/pgrete/test_reflect.mp4
- the reflection test looks like:

test_reflect.mp4

Regarding the user boundary conditions:
In principle, I'd like to test all allowed permutations of boundary conditions as that combination of user and outflow threw me off in first place.
From a design point of view, we'd have to extend the advection example with 6 different user functions.
I imagine those just setting the advected field to a non-zero value and let the now implemented check in FillDerived/SquareIt handle it.
Do you think it's worth adding this change here or should we merge this sooner than later and take care of it in a separate PR?

pgrete · 2021-09-19T11:46:42Z

Ping @Yurlungur for final re-review of latest changes as discussed on Thursday.

Yurlungur

LGTM @pgrete some minor comments below. Really fantastic to have this test in place, along with the reduced code.

Yurlungur · 2021-09-20T21:23:40Z