Provide NPH data in physical boundaries to advection calculation

[mbkuhn]

Summary

Changes requested in conjunction with AMReX-Hydro development. For the sake of time-varying boundary conditions, passing the advected variable data at n+1/2 to the advection routine.

Pull request type

Please check the type of change introduced:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

Checklist

The following is included:

• new unit-test(s)
• new regression test(s)
• documentation for new capability

This PR was tested by running:

• the unit tests
  • on GPU
  • on CPU
• the regression tests
  • on GPU
  • on CPU

[mbkuhn]

Would appreciate help in confirming that this does what it's supposed to do.
Is this automatically covered by the rankine reg test, or do we need to add a new reg test?
Haven't checked the existing reg tests yet.

[asalmgren]

This looks good to me. One change you could make for efficiency is to only fill at most one ghost cell of the nph arrays (filling more is not wrong just possibly most costly).

[marchdf]

I am seeing this:

The following tests FAILED:
          7 - abl_godunov_mpl (Failed)
          8 - abl_godunov_mpl_amr (Failed)
         86 - rankine (Failed)
         87 - rankine-sym (Failed)
         90 - freestream_godunov_inout (Failed)
        116 - abl_bndry_input_native (Failed)
        118 - abl_bndry_input_amr_native (Failed)
        119 - abl_bndry_input_amr_native_mlbc (Failed)
        120 - abl_godunov_forcetimetable (Failed)
        121 - abl_bndry_input (Failed)
        122 - abl_bndry_input_amr (Failed)
        123 - abl_bndry_input_amr_inflow (Failed)

with pretty high diffs. For example:

"rankine-sym" start time: Sep 11 10:44 MDT
Output:
----------------------------------------------------------

            variable name            absolute error            relative error
                                        (||A - B||)         (||A - B||/||A||)
 ----------------------------------------------------------------------------
 level = 0
 density                                          0                         0
 gpx                                 0.001190880702             0.01261228852
 gpy                                0.0001747354762            0.001575471089
 gpz                                8.792966355e-16              0.0133761189
 mu_turb                             0.005611814082            0.004707592218
 p                                    0.09870987147            0.004146244138
 temperature                         3.97903932e-13            1.32634644e-15
 temperature_mueff                    0.01683712596            0.004707573415
 velocityx                          0.0005933946672            5.48743994e-05
 velocityy                           0.004839889088             0.00294386984
 velocityz                          2.719468706e-15            0.001900402607
 velocity_mueff                      0.005611814082            0.004707552728

[mbkuhn]

On the reg test results: we definitely have some concerns, mainly about the bndry_input cases. uncertain about the freestream_godunov_inout as well.

• MPL always gets diffs when there is a different number of fillboundary calls, because it relies on randomness
• rankine and rankine-sym should have diffs because they do have a time-dependent boundary condition
• Not sure about freestream_godunov_inout, but this is related to the capability that led to this change
• All of the abl_bndry_input cases fail (abl_godunov_forcetimetable also takes boundary planes as input), because the use of boundary planes has its own pathway for filling the boundary cells, and using nph data circumvents/replaces that. (bad!)

[mbkuhn]

Summary of boundary plane problem:

• During initialization of restart, fillpatch_state_variables is called. This fills the boundary cells using the boundary plane’s populate_data function. The time argument to this fillpatch is new_time(), but this is the same as current_time() during the initialization. In the abl_godunov_forcetimetable reg test, this time is 2.5 s.
• On the first time step of the simulation (labelled as step 6 in the same reg test), the data within the physical boundary belongs to t = 2.5s. With the fillphysbc call included in this PR, the data within the physical boundary is updated to new_time() (because that is hard-coded into ABLFillInflow::fillphysbc). At this point, new_time() is 2.9 s (in the same reg test).
• I believe this is the source of the diffs. However, I can’t directly confirm this by plugging in current_time() instead of new_time() because after the first time step has begun, the tinterp() variable within ABLBoundaryPlane has been updated, meaning that the old data is no longer available, tripping an assertion in ABLBoundaryPlane::populate_data()
• There are calls to fillphysbc prior to the advection term calculation, but these modify the boundary values of the new state of the advected variable (as it is used for calculating diffusion terms), and the new state of the advected variable is not used for calculating the advection term

[mbkuhn]

Potential steps forward depend on the nature of the boundary planes in time. I don't fully understand the setup now and whether there is a justification for not using NPH times.

[mbkuhn]

Plan is to update NPH bcs at beginning of timestep, prior to moving on to new_time. will require nph state for velocity, which is part of the plan for another feature anyway.

[mbkuhn]

Ok, I think I have everything we need here in this branch. It took a bit more work than expected. There's one more thing that may need to be navigated for compatibility with Ilker's PR.

However, this PR has gotten bigger, and there is a mix of things that should and shouldn't cause diffs. To be thorough, it would probably be best to split this into 2 PRs, with those changes divided. Also doesn't help that our computational resources are down now and it's harder to test.