Skip to content

Conversation

@github-actions
Copy link
Contributor

Automated via .github/workflows/weekly_update.yml.

@github-actions github-actions bot added the component: third party Changes in WarpX that reflect a change in a third-party library label Oct 20, 2025
@EZoni EZoni self-requested a review October 20, 2025 16:13
@EZoni EZoni self-assigned this Oct 20, 2025
@EZoni
Copy link
Member

EZoni commented Oct 20, 2025

I think this needs AMReX-Codes/pyamrex#491 first.

@EZoni
Copy link
Member

EZoni commented Oct 20, 2025

For some reason some compilers started complaining about the fact that particle_shape may be used uninitialized here:

// ensure assumption holds: we read the fields in the interpolation kernel as they are,
// without further communication of guard/ghost/halo regions
int particle_shape;
const ParmParse pp_algo("algo");
utils::parser::getWithParser(pp_algo, "particle_shape", particle_shape);

I know that I can simply fix it by initializing it to, say, 0.

However, this made me realize that when we parse the same parameters multiple times in different parts of the code, we don't always duplicate all the consistency checks that should be applied...

For example, we do this

// ensure assumption holds: we read the fields in the interpolation kernel as they are,
// without further communication of guard/ghost/halo regions
int particle_shape;
const ParmParse pp_algo("algo");
utils::parser::getWithParser(pp_algo, "particle_shape", particle_shape);

but also this

warpx/Source/WarpX.cpp

Lines 1423 to 1439 in edbc4da

int particle_shape;
if (!species_names.empty() || !lasers_names.empty()) {
if (utils::parser::queryWithParser(pp_algo, "particle_shape", particle_shape)){
WARPX_ALWAYS_ASSERT_WITH_MESSAGE(
(particle_shape >= 1) && (particle_shape <=4),
"algo.particle_shape can be only 1, 2, 3, or 4"
);
nox = particle_shape;
noy = particle_shape;
noz = particle_shape;
}
else{
WARPX_ABORT_WITH_MESSAGE(
"algo.particle_shape must be set in the input file:"
" please set algo.particle_shape to 1, 2, 3, or 4");
}

which has many more guard rails.

@ax3l @WeiqunZhang @lucafedeli88
This is error prone potentially, or, at least, it may lead to finding the same bug multiple times, if there is a bug related to if/when/how we parse a paremeter. It sounds somewhat related to the discussion you were having on Slack, regarding #6283.

Luca: Hi, since in the last developers' meeting we mentioned that using ParmParse more often as a unique source of truth for certain parameters could be a way to avoid using static variables of the WarpX class deep in the code, I've started experimenting with three static variables dealing with communications: #6283. The PR removes them from the WarpX class, replacing them in the code with calls to free functions that in turn read the parameters from the inputfile. I would like to know if you like this solution.

Weiqun: What's rational for replacing WarpX static variables with ParmParse? If one regards static variables in a class as evil, reading parameters from a file is even worse. Note that parmparse parameters are stored as variables in an anonymous namespace.

Axel: The idea is to later on use an AMReX stack to differentiate the different ParmParse storages. And to not store control information twice. For interactive use, it is easier to store it once (i.e. WarpX in Jupyter notebooks). In ImpactX, we thus just derive all control from ParmParse, so that we can offer inputs files and interactive use at the same time (e.g, being able to change algorithms used after a certain time step, etc.). Luca, your PR looks good to me. In essence you add a slick function for algorithmic control that can be used everywhere as a one-liner, where a decision on the algorithm needs to be made. Makes sense to me. For very simple options, one can just use ParmParse directly.

@EZoni EZoni requested review from ax3l and lucafedeli88 October 21, 2025 16:55
@EZoni EZoni removed their assignment Oct 21, 2025
@EZoni EZoni enabled auto-merge (squash) October 22, 2025 00:13
@EZoni EZoni merged commit 8c1f985 into development Oct 22, 2025
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: third party Changes in WarpX that reflect a change in a third-party library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants