Implementing k-eps and k-omega RANS models

[marchdf]

Co-authored-by: Jeremy Melvin [email protected]

[jamelvin]

Looks good!

[marchdf]

How is this looking for everyone? I know it's a big one but I've got two more of these kind of PRs sitting in the wing... I tried to break all the work we did into smaller chunks but smaller doesn't mean small ;)

[psakievich]

I'm about halfway through it. I'll try to finish in the next day or so.

[marchdf]

Thanks, @psakievich, for taking the time to look at this!

[psakievich]

Overall looking good, but I'm running out of steam on this first pass. I made it up to the Wilcox files. I'll post what I've seen so far so you can make a few changes.

Main requests are to clean up the few raw pointer instances, and double check that this works on device. I've noted a few syncs that looked like they were missing.

[psakievich]

FYI the modify_on_device flag is set inside field_copy so this is unnecessary. It is a null op though so it doesn't matter. Might be worth adding a sync call on tdrN though. @alanw0 is there a reason we don't do a sync on the src field inside copy_field too?

[marchdf]

Thanks for taking the time! Great catches, all. I really like the enum class idea and might actually do a separate PR for that. I will take care of your requested changes.

[marchdf]

I am not sure why the unittest failed on the latest commit? Looking into that now.

[marchdf]

For reasons I don't understand yet, the unique_ptr edits for tkeEqSys_ (and others, see https://github.com/marchdf/nalu-wind/blob/k-eps/src/ShearStressTransportEquationSystem.C#L70) is causing a segfault in the unittests:

[----------] 20 tests from SSTKernelHex8Mesh
[ RUN      ] SSTKernelHex8Mesh.StreletsUpwindComputation
Process 47428 stopped
* thread #1, name = 'unittestX', stop reason = signal SIGSEGV: invalid address (fault address: 0x331)
    frame #0: 0x0000000000000331
error: memory read failed for 0x200
(lldb) bt
* thread #1, name = 'unittestX', stop reason = signal SIGSEGV: invalid address (fault address: 0x331)
  * frame #0: 0x0000000000000331
    frame #1: 0x0000000000b3a23f unittestX`sierra::nalu::EquationSystems::~EquationSystems() + 47
    frame #2: 0x0000000000be4cfc unittestX`sierra::nalu::Realm::~Realm() + 2060
    frame #3: 0x0000000000be5189 unittestX`sierra::nalu::Realm::~Realm() + 9
    frame #4: 0x0000000000beb5b2 unittestX`sierra::nalu::Realms::~Realms() + 50
    frame #5: 0x0000000000c058da unittestX`sierra::nalu::Simulation::~Simulation() + 26
    frame #6: 0x0000000000723f52 unittestX`unit_test_utils::HelperObjects::~HelperObjects() + 306
    frame #7: 0x00000000008aca98 unittestX`sierra::nalu::SSTKernelHex8Mesh_StreletsUpwindComputation_Test::TestBody() + 5336
    frame #8: 0x00007ffff10d2bad libgtest.so.13`void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 61
    frame #9: 0x00007ffff10be51e libgtest.so.13`testing::Test::Run() + 206
    frame #10: 0x00007ffff10be675 libgtest.so.13`testing::TestInfo::Run() + 309
    frame #11: 0x00007ffff10be76c libgtest.so.13`testing::TestSuite::Run() + 204
    frame #12: 0x00007ffff10c8aa3 libgtest.so.13`testing::internal::UnitTestImpl::RunAllTests() + 1171
    frame #13: 0x00007ffff10c8cd8 libgtest.so.13`testing::UnitTest::Run() + 184
    frame #14: 0x000000000058fae2 unittestX`main + 450
    frame #15: 0x00007fffef411555 libc.so.6`__libc_start_main + 245
    frame #16: 0x00000000006e4833 unittestX`_start + 41

I am wondering if there's some kind of conflict between, e.g. ShearStressTransportEquationSystem.C:

  // push back EQ to manager
  realm_.push_equation_to_systems(this);

and in EquationSystems.C

  for (size_t ie = 0; ie < equationSystemVector_.size(); ++ie)
    delete equationSystemVector_[ie];

[psakievich]

Hmm I'm not immediately seeing why this is failing. It seems like all the unique_ptr's are locally owned and the copy operation is on pointer. There is something about the clean up that is not good though. This could be related to issues I want to address in #997. It's okay if you want to drop the unique_ptr from this PR for now. I don't like doing it, but I don't have the cycles to work through this failure right now. The fact that all the EquationSystem's need an EquationSystems in their constructor seems to indicate some tangled interdependencies.

[marchdf]

Ok I will drop for now. Thanks for looking at this.

[psakievich]

Sorry it took me so long to get back to the review. Thanks for making the changes and going on a unique_ptr adventure with me. I looked at it closer and it will take a bigger (but easy) refactor to make those unique_ptr's. I will do that in a separate PR soon.