search

Exawind / amr-wind

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

Fllc non uniform #1045

Closed tonyinme closed 1 month ago

tonyinme commented 1 month ago

Summary

Non-uniform distribution of points for the FLLC to reduce computational cost. This code will create a non-uniform distribution of actuator points based on the condition that epsilon divided by actuator spacing is less that a specified value. This is ready for review.

Pull request type

Please check the type of change introduced:

Checklist

The following is included:

This PR was tested by running:

Additional background

Issue Number:

tonyinme commented 1 month ago

Thanks @psakievich for your comments. I have addressed most of them in the latest version. This new version is ready for review.

mbkuhn commented 1 month ago

Hey @tonyinme, this doesn't look very ready for review with all of the commented/uncommented print statements littered throughout. Are you wanting it to be checked conceptually before cleaning it up? Also, if you apply the clang formatting, that will enable the PR to go through the rest of the CI checks.

If you're still actively testing it, too, that's fine; just let us know.

tonyinme commented 1 month ago

@mbkuhn this should be ready to review. I think I forgot to push the latest version but it is now all in.

mbkuhn commented 1 month ago

@tonyinme I looked at the merge conflict, and it seems pretty simple to fix. I think the conflict came from an effort to use amrex arrays. Also, I don't see "assert(" elsewhere in the code, so I think the amrex version is probably preferable. I'll make comments directly for those changes.

mbkuhn commented 1 month ago

There's still a failing check because the FLLC unit tests don't pass in debug mode. I'm not sure exactly what the solution is, but the expectations may have changed (needing a modification to the unit test), or there needs to be an improvement to the FLLC code itself.

tonyinme commented 1 month ago

There's still a failing check because the FLLC unit tests don't pass in debug mode. I'm not sure exactly what the solution is, but the expectations may have changed (needing a modification to the unit test), or there needs to be an improvement to the FLLC code itself.

I see, I think the problem comes from the nonuniform flag being true.

There are 2 options.

  1. Leaving the default of the nonuniform flag as false
  2. Passing that flag to the unit-test as false
psakievich commented 1 month ago

I see, I think the problem comes from the nonuniform flag being true.

There are 2 options.

  1. Leaving the default of the nonuniform flag as false
  2. Passing that flag to the unit-test as false

I don't think either of those are acceptable options. The failure looks like an index out of range error which means there is a memory leak with the non-uniform flag. 

[----------] 1 test from TestFLLCData
[ RUN      ] TestFLLCData.data_initializes_with_cviews
terminate called after throwing an instance of 'std::runtime_error'
  what():  Assertion `i < (this->std::vector<T, Allocator>::size())' failed, file "/home/runner/work/amr-wind/amr-wind/submods/amrex/Src/Base/AMReX_Vector.H", line 35

The proper thing to do is to fix the unit test to make it work if you expect the nonuniform flag to be a production feature, or pass it as false then write a unit-test that works with the non-uniform flag.

You should probably do the second (or parameterize the test to test both conditions) b/c we want to know that both code paths work.

tonyinme commented 1 month ago

The unit test has now been fixed and a new unit test was added for the nonuniform version. Thanks @psakievich for helping with the unit test.

marchdf commented 1 month ago

@tonyinme and @psakievich I am seeing:

33/91 Testing: act_fixed_wing_fllc
33/91 Test: act_fixed_wing_fllc
Command: "/usr/bin/sh" "-c" "/data/ssd1/software/2024-05-01/opt/software/linux-rocky8-zen2/gcc-12.3.0/mpich-4.2.1-ujwsut4nrhjde47hw6dxp5ato3cnomic/bin/mpiexec -n 4  /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/amr_wind  /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/test/test_files/act_fixed_wing_fllc/act_fixed_wing_fllc.inp time.max_step=10 io.plot_file=plt time.plot_interval=10 amrex.the_arena_is_managed=0 a\
mrex.abort_on_out_of_gpu_memory=1 amrex.signal_handling=1 amrex.fpe_trap_invalid=1 amrex.fpe_trap_zero=1 amrex.fpe_trap_overflow=1 > act_fixed_wing_fllc.log && cp -R /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/test/test_files/act_fixed_wing_fllc/plt00010 /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/golds/tmp/Linux/GNU/12.3.0/act_fixed_wing_fllc/ && /data/ssd1/software/2024-05-01/opt/software/linux-rocky8-ze\
n2/gcc-12.3.0/mpich-4.2.1-ujwsut4nrhjde47hw6dxp5ato3cnomic/bin/mpiexec -n 4  /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/submods/amrex/Tools/Plotfile/amrex_fcompare  /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/golds/current/Linux/GNU/12.3.0/act_fixed_wing_fllc/plt00010 /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/test/test_files/act_fixed_wing_fllc/plt00010"
Directory: /mnt/vdb/home/mhenryde/exawind/source/amr-wind/build/test/test_files/act_fixed_wing_fllc/
"act_fixed_wing_fllc" start time: May 22 10:13 MDT
Output:
----------------------------------------------------------

            variable name            absolute error            relative error
                                        (||A - B||)         (||A - B||/||A||)
 ----------------------------------------------------------------------------
 level = 0
 actuator_src_termx                   0.00010297905           0.0009632885327
 actuator_src_termy                               0                         0
 actuator_src_termz                  0.002462548222            0.001067099333
 density                                          0                         0
 gpx                                0.0003462567195            0.001085198849
 gpy                                0.0003116651018            0.001270929168
 gpz                                 0.001022497373            0.001100755329
 p                                   0.002045025522            0.001114726229
 velocityx                          9.023380168e-05           8.912046896e-06
 velocityy                          9.056170363e-05           0.0007651859972
 velocityz                          0.0004328949134           0.0007423132165
<end of output>
Test time =   1.34 sec
----------------------------------------------------------
Test Failed.
"act_fixed_wing_fllc" end time: May 22 10:13 MDT
"act_fixed_wing_fllc" time elapsed: 00:00:01

because of this commit. Do you expect this? If so, I will just rebless my golds. Just double checking that this is intended.

mbkuhn commented 1 month ago

I think this makes sense because there is a new default behavior. However, before we rebless, we should probably confirm that the old pathway, which is still in the code, introduces no diffs. would you be able to test it with Actuator.FixedWingLine.fllc_nonuniform = false added to the input file?

tonyinme commented 1 month ago

Ah, yes, this is expected. If we want to default to the original test value, we can set this flag to false in the test.

psakievich commented 1 month ago

I think a rebless is warranted over changing the test. The default behavior is not really intended to be used in production research

marchdf commented 1 month ago

Ok cool, yes tests pass with Actuator.FixedWingLine.fllc_nonuniform = false so I will rebless. Thanks!