Generalize twtstight for arbitrary beta0 and code refactoring #5174
Conversation
|
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? |
|
Yes, that should be quite simple. |
|
Great, thanks! |
|
@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 |
|
@chillenzer Please leave the draft-tag for now until Monday. It is late and it is a good idea to have some time pass, |
|
@chillenzer I actually have further minor changes to make the PR more round. I will submit them soon. |
BeyondEspresso
force-pushed
the
topic-bugfix-twtstight-dy-dt-lengths
branch
from
8881fc8
to
cbeff6b
Compare
BeyondEspresso
changed the title
|
@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. |
There was a problem hiding this comment.
Very well annotated, thanks for the great guidance. Still there are a few things to discuss. Also, I can't say anything about the actual physics formula. Needs a check from @steindev .
include/picongpu/fields/background/templates/twtstight/BField.hpp
Outdated
Show resolved
Hide resolved
| * | ||
| * For moving window simulations, POSITION for the YMax side can be located outside the initially | ||
| * simulated volume. In this case, parts of the generation surface outside of the currently simulated | ||
| * volume is are treated as if they had zero incident field and it is user's responsibility to apply a |
There was a problem hiding this comment.
| * volume is are treated as if they had zero incident field and it is user's responsibility to apply a | |
| * volume are treated as if they had zero incident field and it is user's responsibility to apply a |
There was a problem hiding this comment.
I'll change this.
| auto const phiT = float_T(math::abs(phi)); | ||
| float_T sinPhi; | ||
| float_T cosPhi; | ||
| pmacc::math::sincos(phiT, sinPhi, cosPhi); |
There was a problem hiding this comment.
Sorry for my ignorance but could you please quickly explain if math and pmacc::math are different namespaces. Also, given that those are frequently used, a using directive might be beneficial for this function.
There was a problem hiding this comment.
During the development of this code, namespaces have changed. I guess that this is a remainder that can be removed now. I'll check.
include/picongpu/fields/background/templates/twtstight/BField.tpp
Outdated
Show resolved
Hide resolved
| @@ -262,15 +242,12 @@ namespace picongpu | |||
| /* To avoid underflows in computation, fields are set to zero | |||
| * before and after the respective TWTS pulse envelope. | |||
| */ | |||
| if(math::abs(y - z * math::tan(phiT / float_T(2.0)) - (cspeed * t)) > (numSigmas * tauG * cspeed)) | |||
| if(math::abs(y - z * tanAlpha - (beta0 * cspeed * t)) > (numSigmas * tauG * cspeed)) | |||
| return float_T(0.0); | |||
|
|
|||
| /* Calculating shortcuts for speeding up field calculation */ | |||
There was a problem hiding this comment.
I think you're perfectly fine to assume that the compiler would be capable of figuring this out for itself and will happily ignore if these are present or not, using its own heuristics instead. The only benefit of these could be simplifying formulae later on. You can decide for yourself if for any or all of these this purpose is fulfilled.
There was a problem hiding this comment.
Was this comment meant for a different line? Without this if-statement, the code produces NaNs due to underflow. We already were severely bitten by this. The function of this statement corresponds to the "laser window" of a standard Gaussian laser.
There was a problem hiding this comment.
Sorry, always forget how badly GH renders comments on a single line. This was supposed to be a comment on "Calculating shortcuts for speeding up field calculation".
There was a problem hiding this comment.
I'm not quite happy with the three functions being mostly duplications of one another with a few switches of sin and cos. It is well annotated and I could follow along quite well but it would be a huge pain to change every definition three times (as I believe you have experienced during this refactoring). Could we make this more elegant? One option could be to extract some common methods for computing the basics at the top, potentially using structured bindings or a shallow struct for the return value. Alternatively, one could think about merging the functions computing and returning all three components (if they are not needed separately).
There was a problem hiding this comment.
I fully agree, as I also spend some thought on it. However, I always postponed it until later. Let us discuss the most suitable approach offline (or in a VC call).
There was a problem hiding this comment.
@chillenzer @BrianMarre I just successfully compiled a small proof of concept for the structured bindings within twtstight using 3 variables on both NVIDIA and AMD GPUs.
There was a problem hiding this comment.
Skimming through this, the same comments apply as for the B field. Even more so, I think we should extract common functions, so we don't have a 6-fold code duplication of round about 70 lines.
There was a problem hiding this comment.
See above. Let us discuss this before I make changes.
|
@BrianMarre should probably follow the developments here after we've done a similar refactoring with his code. |
Have you actually added said test yet or is this still on your todo list? |
|
@chillenzer Yes, adding the test is still on my todo list. |
psychocoderHPC
added
the
refactoring
|
@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. |
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.