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.

Sign up for GitHub

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

Jump to bottom

Diagnostics: Fix Position Selection #4914

Merged
merged 6 commits into from
May 12, 2024
Merged

Diagnostics: Fix Position Selection #4914

merged 6 commits into from
May 12, 2024

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented May 6, 2024 •
edited
Loading

Follow-up to #4653

@ax3l ax3l added bug Something isn't working bug: affects latest release Bug also exists in latest release version component: diagnostics all types of outputs labels May 6, 2024
@ax3l ax3l marked this pull request as ready for review May 6, 2024 15:41
@ax3l ax3l requested review from RemiLehe and kale-j May 6, 2024 15:41
@ax3l ax3l mentioned this pull request May 6, 2024
Closed
@ax3l ax3l added the component: openPMD openPMD I/O label May 6, 2024
@ax3l ax3l force-pushed the topic-diag-no-user-positions branch 6 times, most recently from f172beb to 4531519 Compare May 6, 2024 15:51
@ax3l ax3l mentioned this pull request May 6, 2024
Closed
kale-j
kale-j approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@kale-j kale-j left a comment

Choose a reason for hiding this comment

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

This is now much clearer!
I tested in 2D and 3D -- seems to work as expected

@RemiLehe
Copy link
Member

RemiLehe commented May 7, 2024

Thanks @ax3l ! Would you have time to check the clang-tidy error?

@ax3l
Copy link
Member Author

ax3l commented May 7, 2024

@kale-j thank you for testing! Glad this works well! Thanks again for reporting this ✨

@RemiLehe absolutely - fixed now :)

@ax3l ax3l mentioned this pull request May 8, 2024
Open
@ax3l ax3l force-pushed the topic-diag-no-user-positions branch 3 times, most recently from 1f45292 to 20addda Compare May 8, 2024 21:42
ax3l added 5 commits May 8, 2024 19:41
@ax3l
Diagnostics: Warn w/o Positions
7d51f86 
If a user specifies `<diag_name>.<species_name>.variables`
without including particle position arguments we now warn.
@ax3l
openPMD: Skip 0-positions for deselected comps
cc28d20
@ax3l
Clang-Tidy: Braces
e848d8c
@ax3l
Update Examples
2075825
@ax3l
RZ: x,y,z
c734171
@ax3l ax3l force-pushed the topic-diag-no-user-positions branch from 20addda to c734171 Compare May 9, 2024 02:41
@ax3l
Fix: openPMD Low-D position/positionOffset
b670db8 
Continue to write defaults for coarse and fine
position of omitted dimensions to zero.
@ax3l ax3l force-pushed the topic-diag-no-user-positions branch from f59e17b to b670db8 Compare May 9, 2024 07:53
@ax3l
Copy link
Member Author

ax3l commented May 12, 2024

@RemiLehe when you have a second, can you approve this for the release? :)

@ax3l ax3l enabled auto-merge (squash) May 12, 2024 02:10
@ax3l ax3l disabled auto-merge May 12, 2024 04:34
@ax3l
Copy link
Member Author

ax3l commented May 12, 2024 •
edited
Loading

Merging, so we can do the release 24.05 :)
It is tested by @kale-j and me and only small fixes where left after Remi saw it :)

Follow-ups that we found during testing are documented in #4917

@ax3l ax3l merged commit e6c1eb5 into ECP-WarpX:development May 12, 2024
45 checks passed
@ax3l ax3l deleted the topic-diag-no-user-positions branch May 12, 2024 19:16
ax3l
ax3l commented May 12, 2024
View reviewed changes
Source/Diagnostics/ParticleDiag/ParticleDiag.cpp
std::map<std::string, int> existing_variable_names = pc->getParticleComps();
#ifdef WARPX_DIM_RZ
// we reconstruct to Cartesian x,y,z for RZ particle output
existing_variable_names["y"] = PIdx::theta;
Copy link
Member Author

@ax3l ax3l May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a hack to have y enabled and de-selecting was already broken before.
#4917 will do a general overhaul of all this logic and remove this assignment.

@ax3l ax3l mentioned this pull request May 13, 2024
Merged
Haavaan pushed a commit to Haavaan/WarpX that referenced this pull request Jun 5, 2024
@ax3l @Haavaan
Diagnostics: Fix Position Selection (ECP-WarpX#4914)
4ed1d59 
* Diagnostics: Warn w/o Positions

If a user specifies `<diag_name>.<species_name>.variables`
without including particle position arguments we now warn.

* openPMD: Skip 0-positions for deselected comps

* Clang-Tidy: Braces

* Update Examples

* RZ: x,y,z

* Fix: openPMD Low-D position/positionOffset

Continue to write defaults for coarse and fine
position of omitted dimensions to zero.
@EZoni EZoni mentioned this pull request Jul 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kale-j kale-j kale-j approved these changes

@RemiLehe RemiLehe Awaiting requested review from RemiLehe

Assignees

@RemiLehe RemiLehe

Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working component: diagnostics all types of outputs component: openPMD openPMD I/O
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Particle diagnostic silently writes zero position if x y z is not explicitly specified
3 participants
@ax3l @RemiLehe @kale-j