-
Notifications
You must be signed in to change notification settings - Fork 194
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
BTD-RZ Add multiple modes #3482
BTD-RZ Add multiple modes #3482
Conversation
701e7a2
to
7dffa63
Compare
@RevathiJambunathan ready for rebase :) |
2e0aaea
to
66b395e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, @RevathiJambunathan! Sorry, I decided to jump in as a reviewer to maybe help merge this faster. So far I left only a few minor comments on using WarpX::ncomps
directly where possible, following the spirit of #2951. Unless we think it's unsafe for some reason.
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
This pull request introduces 1 alert when merging 0aded06 into 8f9da8c - view on LGTM.com new alerts:
|
The CI test is failing because After running this with DEBUG, Here is the Backtrace.0.0.txt I am wondering if something about In the meanwhile, I also tried running this test with one RZ mode, and I get the following warning
|
Update : I printed the min/max values of the cell-centered multifab being sliced and it all looks okay. nothing suspicious there, definitely no nans or really small/large numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
I added a few suggestions. I will simultaneously also have a look at the failing test to try to understand better what is happening there.
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
for (int imode = 0; imode < n_rz; ++imode) { | ||
int offset = 1; | ||
// number of components for a given mode | ||
int ncomp = 2; | ||
if (imode == 0) { | ||
offset = 0; | ||
// Mode 0 has only real part, thus ncomp = 1 | ||
ncomp = 1; | ||
} | ||
for (int icomp = 0; icomp < ncomp; ++icomp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we replace the whole code above by a loop over components?
for (int imode = 0; imode < n_rz; ++imode) { | |
int offset = 1; | |
// number of components for a given mode | |
int ncomp = 2; | |
if (imode == 0) { | |
offset = 0; | |
// Mode 0 has only real part, thus ncomp = 1 | |
ncomp = 1; | |
} | |
for (int icomp = 0; icomp < ncomp; ++icomp) { | |
for (int mode_comp_offset = 0; mode_comp_offset < nr_comp; ++mode_comp_offset) { |
// Offset for a given mode, imode, and component, icomp (0-real, 1-imag) | ||
const int mode_comp_offset = 2*imode + offset*(icomp-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If simplify the above loop as suggested, then we can remove these lines.
// Offset for a given mode, imode, and component, icomp (0-real, 1-imag) | |
const int mode_comp_offset = 2*imode + offset*(icomp-1); | |
// Offset for a given mode, imode, and component, icomp (0-real, 1-imag) | |
const int mode_comp_offset = 2*imode + offset*(icomp-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep it this way since it accesses the other fields easily
Or rather it has the same "structure" as for cartesian
e_lab = gamma_boost * ( arr(i, j, k, 0)
+ beta_boost * clight * arr(i, j, k, 4) );
b_lab = gamma_boost * ( arr(i, j, k, 4)
+ beta_boost * inv_clight * arr(i, j, k, 0) );
and for RZ we have
e_lab = gamma_boost * ( arr(i, j, k, n_rcomps*0 + mode_comp_offset)
+ beta_boost * clight * arr(i, j, k, n_rcomps*4+ mode_comp_offset) );
b_lab = gamma_boost * ( arr(i, j, k, n_rcomps*4 + mode_comp_offset)
+ beta_boost * inv_clight * arr(i, j, k, n_rcomps*0 + mode_comp_offset) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I am open to changing this as well.
Need to think about this one -- I think I can find a way to add in the suggested change in the loop while keeping a similar logic, as long as it does not get too complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay this is done :)
Thank you so much @RemiLehe
Your suggestion is more clean and readable! Thanks!
From Axels' review Co-authored-by: Axel Huebl <[email protected]>
suggestion from Edoardo Co-authored-by: Edoardo Zoni <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Remi Lehe <[email protected]>
for more information, see https://pre-commit.ci
f6ffadd
to
123eea6
Compare
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp
Outdated
Show resolved
Hide resolved
* cell center BTD functors for RZ with openpmd * add RZ modes to output varnames too * update varnames once and set map for RZ fields in BTfunctor * clean commented line * Apply suggestions from code review From Axels' review Co-authored-by: Axel Huebl <[email protected]> * adding comments, doxygen, and clean-up * adding mulitple modes to RZ BTD * fix comment * fix bug using 2*nrz-1 , instead of nrz * add comments and clean up * fix typo * 1D * WARPX_DIM_XZ instead of 2D * Apply suggestions from code review suggestion from Edoardo Co-authored-by: Edoardo Zoni <[email protected]> * remove BTD RZ field warning that does not apply anymore * BTD rz test for field using laser antenna * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * file starts with analysis * change analysis * fix typo * fix rz input so all snapshots are filled * remove plt from analysis script * initialize cell-centered data to 0 so that guard cells are initialized * Remi's suggestions Co-authored-by: Remi Lehe <[email protected]> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * AllocInitMultifab to add mf to maps of mfs * fix path to analysis script * analysis script executable * a better and succint for loop * unused var * Update Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp Co-authored-by: Remi Lehe <[email protected]> * fix unused var * add python path Co-authored-by: Axel Huebl <[email protected]> Co-authored-by: Edoardo Zoni <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Remi Lehe <[email protected]>
To be merged after #3350
input file used for testing on summit :
inputs.txt