Fix dY and dT symmetry scales in twtstight

[BeyondEspresso]

The symmetry scales dY and dT are used to maintain longterm numerical stability by exploiting a spatio-temporal symmetry in the twtstight laser pulse. There has been a bug in dY and dT. Since their equations were unnecessarily convoluted, I also refactored them in the fix. The equations now pass the dY-dT-symmetry test.

[chillenzer]

Without having had much more than a quick glance on it, would it be an option to add that test you were talking about as a unit/end-to-end test?

[BeyondEspresso]

Yes, that should be quite simple.

[chillenzer]

Great, thanks!

[BeyondEspresso]

@chillenzer @steindev The small PR has grown quite a bit. After applying the original updates I noticed that the twtstight-implementation does not work for beta0 != 1.0, which is not the normal usecase, but it is in the interface and actually nice to have. I updated the laser equations to now also include beta0. I tested the new implementation that compared to my model implementation correctly generates the old (beta0=1.0) and now also new results (beta0 != 1.0). While applying the changes, I seized the opportunity for some reductions in boiler plate code that still existed from previous, now outdated, twts implementations.

[BeyondEspresso]

@chillenzer Please leave the draft-tag for now until Monday. It is late and it is a good idea to have some time pass,

[BeyondEspresso]

@chillenzer I actually have further minor changes to make the PR more round. I will submit them soon.

[BeyondEspresso]

@chillenzer @steindev OK, I rebased my PR and completed my generalization and refactoring. Additionally, I successfully tested the code in an actual PIConGPU run. The draft-tag can be removed now.

[chillenzer]

@BrianMarre should probably follow the developments here after we've done a similar refactoring with his code.

[chillenzer]

Without having had much more than a quick glance on it, would it be an option to add that test you were talking about as a unit/end-to-end test?

Have you actually added said test yet or is this still on your todo list?

[BeyondEspresso]

@chillenzer Yes, adding the test is still on my todo list.

[BeyondEspresso]

@psychocoderHPC @chillenzer Ok, here is the refactored version. To reduce duplicate code I moved the implementation into the new template class TWTSTight and specialized for the respective E- and B-Field implementation. In the math, I also did a separation of complex and float-numbers via reordering and brackets according to @psychocoderHPC suggestions. Due to the new struct, I renamed twtstight.tpp to TWTSTight.tpp. In total this results in a code reduction by almost 600 LOCs.

[BeyondEspresso]

@chillenzer Before you review the code, I need to review it myself, as there are changes that were not intended.

[BeyondEspresso]

@chillenzer Should be OK now. In addition to updating attributions and fixing typos, there was a missing ", which failed the CI.

[chillenzer]

Nice! With this refactoring, the PR actually reduces the lines of code in the repo while still adding new functionality if I understood correctly.

[BeyondEspresso]

That is correct.

[BeyondEspresso]

@chillenzer Now I also added the unit test to the PR.

[BeyondEspresso]

@chillenzer @psychocoderHPC I adressed your comments. There were quite some changes in how the helper variables are handled. Now there are two arrays of variables part of the data members, which makes everything more concise. The test case compiles, runs and passes on Frontier.

[BeyondEspresso]

@psychocoderHPC @chillenzer The core dump problem has not gone away by changing to CHECK().

/builds/hzdr/crp/picongpu/share/picongpu/unit/TWTSTight.cpp(177): internal error: assertion failed at: "error.c", line 7967 in diagnostic_pragma
1 catastrophic error detected in the compilation of "/builds/hzdr/crp/picongpu/share/picongpu/unit/TWTSTight.cpp".
Compilation aborted.
Aborted (core dumped)
make[2]: *** [CMakeFiles/UnitTest-TWTSTight.cpp-2D.dir/build.make:76: CMakeFiles/UnitTest-TWTSTight.cpp-2D.dir/TWTSTight.cpp.o] Error 134

Other compilers compiled this without any issue.

[chillenzer]

Okay. I found the offending piece of code using gcc 11.3.0 and CUDA 11.6.0. But I'm not sure if we can do anything about this: The specific combination of compiler and CUDA version fails at the following line:

#pragma nv_diagnostic push

which can be reproduced by putting this anywhere in the code. This line is inserted by Catch2 in thirdParty/catch2/src/catch2/internal/catch_test_macro_impl.hpp which defines INTERNAL_CATCH_TEST (which is more or less what CHECK and REQUIRE etc. compile to) through CATCH_INTERNAL_START_WARNINGS_SUPPRESSION (from thirdParty/catch2/src/catch2/internal/catch_compiler_capabilities.hpp).

Catch2 claims in its changelog that they have fixed warning suppression for NVIDIA in 3.3.0 (and we're using 3.4.0) but apparently that doesn't apply to our compiler/CUDA combination. I tried updating to the latest Catch2 (3.7.1) but that doesn't change anything.

Things that we could do here:

• Disable this test for this specific combination.
• Hack/patch/customise our Catch2 to remove this line.
• Try to make Catch2 believe it's on a different compiler (the host compiler might be an option).

[psychocoderHPC]

Thanks for investigating it. Can we please disable the test in the source code for this compiler. There are alpaka macros to control the version.

[chillenzer]

@BeyondEspresso, I've suggested the corresponding patch. Feel free to adopt it. @psychocoderHPC, you should probably have a look if that macro's what you expected. Don't think alpaka has anything like that (except for CI specific macros).

[BeyondEspresso]

@chillenzer Thanks for the investigation and suggested code change. Let us see whether the updated PR now passes all CI hoops.