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

Particle diagnostic silently writes zero position if x y z is not explicitly specified #4841

Closed
kale-j opened this issue Apr 9, 2024 · 9 comments · Fixed by #4914
Closed
Assignees
Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working component: diagnostics all types of outputs

Comments

@kale-j
Copy link
Contributor

kale-j commented Apr 9, 2024

One of the changes in the last few months (some time between December and March) has affected the default behavior of the output for the particle data diagnostic. The new behavior seems unexpected and is not documented.

In a Full diagnostic, consider
<diag_name>.<species_name>.variables = w

Previous behavior:
Particle position is written in addition to the weight
-- this is what the documentation indicates should happen

Current behavior:
Zeros are written for the particle position regardless of where the particles actually are
The actual particle position is only written if the desired positions are explicitly specified, i.e.,
<diag_name>.<species_name>.variables = w x y z

MWE is attached

inputs_3d.txt

@RemiLehe
Copy link
Member

Thanks for the nice reproducer. Could you confirm that #4855 fixes this issue?

@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 Apr 15, 2024
@ax3l ax3l self-assigned this Apr 15, 2024
@ax3l
Copy link
Member

ax3l commented Apr 15, 2024

Discussed on I/O channel:
Particle positions are able to be filtered since the SoA update, similar to all other particle properties.

We will try the following update:

  • skip SetupPos if not needed
  • warn the user if no particle position is written (ok, but might be an accident)

@ax3l
Copy link
Member

ax3l commented Apr 16, 2024

@kale-j Thank you for the report!

Previous behavior:
Particle position is written in addition to the weight
-- this is what the documentation indicates should happen

Can you point me to where the docs still read that? This must have slipped through. We changed this behavior (and the docs) so that positions are treated like any other particle property as well when we transitioned our particles to a pure SoA data structure in the last months.

Current behavior:
Zeros are written for the particle position regardless of where the particles actually are
The actual particle position is only written if the desired positions are explicitly specified, i.e.,

Thanks for the reproducer 🙏 This should simply not write the position when not selected (bug).

@RemiLehe
Copy link
Member

@ax3l It seems that the documentation for <diag_name>.<species_name>.variables on this page is not very explicit on this point. It says: "Choices are w for the particle weight and ux uy uz for the particle momenta.", but does not list x, y, z as possible choices...

@ax3l
Copy link
Member

ax3l commented Apr 16, 2024

Thanks, agreed! And also, we have additionally shortcuts position and momentum, if I recall correctly, which each set all three.

@kale-j
Copy link
Contributor Author

kale-j commented Apr 26, 2024

@RemiLehe
#4855 appears to be a fix as far as I can tell

@ax3l
To add to what Remi said about the documentation, I interpreted this line to mean that the particle position is always included regardless of what variables are specified. If you want to not have the position written by default, I would suggest editing this part of the documentation to prevent confusion

If ``<diag_name>.<species_name>.variables = none``, no particle data are written, except for particle positions, which are always included.

@RemiLehe
Copy link
Member

@ax3l Should we merge #4855 now (to fix the existing bug), and later do a follow-up PR, where we would allow users to select whether or not to write positions (desired behavior, eventually).

@ax3l
Copy link
Member

ax3l commented Apr 30, 2024

@kale-j Thanks, argh that slipped through and should have been removed with the recent PR.

@RemiLehe I will give it a try this week to rather fix it as intended. @kale-j please use the patch in the meantime?

@ax3l
Copy link
Member

ax3l commented May 6, 2024

I pushed an update to both outdated docs and diagnostics: #4914

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: affects latest release Bug also exists in latest release version bug Something isn't working component: diagnostics all types of outputs
Projects
None yet
3 participants