Exawind / amr-wind

AMReX-based structured wind solver
https://exawind.github.io/amr-wind
Other
106 stars 83 forks source link

Fix for correct velocity sampling upon initialization (actuator methods) #974

Closed mbkuhn closed 7 months ago

mbkuhn commented 7 months ago

fillpatch velocity field prior to sampling Closes #750

mbkuhn commented 7 months ago

@tonyinme feel free to confirm that this fixes the issue. I got it to work for me.

mbkuhn commented 7 months ago

@marchdf I think this brings up a bigger issue, which is why I'm keeping this as a draft pull request. In many cases, the "initialize_fields" functions for different physics cases do not fill the ghost cells. This particular bug #750 reveals this issue because the particles were sampling some real values in the domain (1.0) and some non-initialized ghost cells (0.0) leading to sampled velocities between 0 and 1 (mostly 0.5).

One option would be to make sure that initialize_fields functions go into the ghost cells. A fill patch operation doesn't really make sense there because those are level-based functions. Instead, I think it makes the most sense for the PDEs to call a fillpatch on their fields as a post_init step, but that is currently only taking place as a post_solve step. Does anything fill the ghost cells prior to the first advance?

tonyinme commented 7 months ago

Thank you @mbkuhn ! I can confirm that the issue is no longer there with this new fix.

mbkuhn commented 7 months ago

@marchdf we have much less of an issue to worry about, actually. I already fixed the lack of a fillpatch prior to advancing with this line: https://github.com/Exawind/amr-wind/blob/main/amr-wind/equation_systems/PDE.H#L76 . However, what we are running into now is the fact that the post_init actions of physics classes happen before the initialize() calls for PDEs. https://github.com/Exawind/amr-wind/blob/main/amr-wind/incflo.cpp#L105 . We could switch the ordering here and see what happens to the reg tests.

marchdf commented 7 months ago

Tests on CPU:

92% tests passed, 7 tests failed out of 89

Label Time Summary:
no_ci         = 17938.43 sec*proc (83 tests)
regression    = 18696.09 sec*proc (88 tests)
unit          =  27.70 sec*proc (1 test)

Total Test time (real) = 527.42 sec

The following tests FAILED:
         31 - act_fixed_wing (Failed)
         32 - act_fixed_wing_fllc (Failed)
         34 - act_flat_plate (Failed)
         36 - uniform_ct_disk (Failed)
         37 - uniform_ct_disk_gaussian (Failed)
         38 - joukowsky_disk (Failed)

And one of those diffs is like:

            variable name            absolute error            relative error
                                        (||A - B||)         (||A - B||/||A||)
 ----------------------------------------------------------------------------
 level = 0
 actuator_src_termx                 1.179564383e-07           2.692688554e-05
 actuator_src_termy                               0                         0
 actuator_src_termz                 2.107577773e-06           3.430702232e-06
 density                                          0                         0
 gpx                                 9.80272009e-06           0.0001179433969
 gpy                                1.824766586e-06           2.788328072e-05
 gpz                                 8.97545978e-06           3.599570741e-05
 p                                  1.107432546e-05           2.256430147e-05
 velocityx                          8.675986287e-06           8.648217839e-07
 velocityy                          5.143160887e-06           0.0001750375179
 velocityz                            3.5619062e-05           0.0002471122068
srun: error: r4i0n8: tasks 0-3: Exited with exit code 1
srun: Terminating StepId=14073363.66
<end of output>
Test time =  58.24 sec
----------------------------------------------------------
Test Failed.
"act_flat_plate" end time: Feb 14 10:46 MST
"act_flat_plate" time elapsed: 00:00:58

But I think we expect this given the change made to the code. Just documenting this here.

marchdf commented 7 months ago

@tonyinme can you check this PR again? @mbkuhn made a couple changes.

mbkuhn commented 7 months ago

@tonyinme can you check this PR again? @mbkuhn made a couple changes.

@tonyinme Tony, it's alright, I checked it myself.

tonyinme commented 7 months ago

Sounds good, I checked and the fix works well with the newer version!