Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize twtstight for arbitrary beta0 and code refactoring #5174

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace picongpu
class BField
{
public:
using float_T = float_64;
using float_T = float_X;
chillenzer marked this conversation as resolved.
Show resolved Hide resolved

/** Center of simulation volume in number of cells */
PMACC_ALIGN(halfSimSize, DataSpace<simDim>);
Expand Down
193 changes: 66 additions & 127 deletions include/picongpu/fields/background/templates/twtstight/BField.tpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace picongpu
class EField
{
public:
using float_T = float_64;
using float_T = float_X;

/** Center of simulation volume in number of cells */
PMACC_ALIGN(halfSimSize, DataSpace<simDim>);
Expand Down
193 changes: 66 additions & 127 deletions include/picongpu/fields/background/templates/twtstight/EField.tpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. Let us discuss this before I make changes.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,10 @@ namespace picongpu
/** To avoid underflows in computation, numsigmas controls where a zero cutoff is made.
* The fields thus are set to zero at a position (numSigmas * tauG * cspeed) ahead
* and behind the respective TWTS pulse envelope.
* Developer note: In case the float_T-type is set to float_X instead of float_64,
* numSigma needs to be adjusted to numSigmas = 6 to avoid numerical issues.
* Developer note: In case the float_T-type is set to float_64 instead of float_X,
* numSigma can be increased to numSigmas = 10 without running into numerical issues.
*/
constexpr uint32_t numSigmas = 10;
constexpr uint32_t numSigmas = 6;
} // namespace twtstight
} // namespace templates
} // namespace picongpu
Expand Down
12 changes: 6 additions & 6 deletions share/picongpu/benchmarks/TWEAC-FOM/cmakeFlags
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
#
# Copyright 2013-2023 Axel Huebl, Rene Widera, Sergei Bastrakov
# Copyright 2013-2024 Axel Huebl, Rene Widera, Sergei Bastrakov, Alexander Debus
#
# This file is part of PIConGPU.
#
Expand Down Expand Up @@ -29,17 +29,17 @@
# - start with zero index
# - increase by 1, no gaps

# use field background for laser generation (will not use incident field)
flags[0]="-DPARAM_OVERWRITES:LIST='-DPARAM_FIELD_BACKGROUND=1'"
# use incident field for laser generation (will not use field background and should be faster)
# use componentwise field calculation (may be faster?)
flags[0]="-DPARAM_OVERWRITES:LIST='-DPARAM_FIELD_BACKGROUND=0;-DPARAM_COMPONENTWISE=1'"

# limit examles if we run CI tests
if [ -z "$PIC_CI_MAINLINE" ] ; then
# use incident field for laser generation (will not use field background and should be faster)
# use full field calculation
flags[1]="-DPARAM_OVERWRITES:LIST='-DPARAM_FIELD_BACKGROUND=0;-DPARAM_COMPONENTWISE=0'"
# use incident field for laser generation (will not use field background and should be faster)
# use componentwise field calculation (may be faster?)
flags[2]="-DPARAM_OVERWRITES:LIST='-DPARAM_FIELD_BACKGROUND=0;-DPARAM_COMPONENTWISE=1'"
# use field background for laser generation (will not use incident field)
flags[2]="-DPARAM_OVERWRITES:LIST='-DPARAM_FIELD_BACKGROUND=1'"
fi

################################################################################
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,29 @@ namespace picongpu
using ZMin = MyProfile;
using ZMax = MyProfile;

// These values are chosen to match the background, have to be changed for higher order field solvers
/** Position in cells of the Huygens surface relative to start of the total domain
*
* The position is set as an offset, in cells, counted from the start of the total domain.
* For the max boundaries, negative position values are allowed.
* These negative values are treated as position at (global_domain_size[d] + POSITION[d][1]).
* It is also possible to specify the position explicitly as a positive number.
* Then it is on a user to make sure the position is correctly calculated wrt the grid size.
*
* Except moving window simulations, the position must be inside the global domain.
* The distance between the Huygens surface and each global domain boundary must be at least
* absorber_thickness + (FDTD_spatial_order / 2 - 1). However beware of setting position = direction *
* (absorber_thickness + const), as then changing absorber parameters will affect laser positioning.
* When all used profiles are None, the check for POSITION validity is skipped.
*
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change this.

* source matching such a case.
*/
constexpr int32_t POSITION[3][2] = {
{1, -1}, // x direction [negative, positive]
{1, -1}, // y direction [negative, positive]
{1, -1} // z direction [negative, positive]
{16, -16}, // x direction [negative, positive]
{16, -16}, // y direction [negative, positive]
{16, -16} // z direction [negative, positive]
};

} // namespace incidentField
Expand Down