picongpu
picongpu copied to clipboard
ComputationalRadiationPhysics
Fix of radiation window for moving simulation.
This introduces a shift of the radiation window winFkt
by globalOffset * cellSize to shift the radiation window
to the moving window.
This should fix #4239.
I believe global in this comment is wrong
https://github.com/ComputationalRadiationPhysics/picongpu/blob/c624c662ea615ed927a9275c4ff3d4786d11dcba/include/picongpu/plugins/radiation/Radiation.kernel#L279-L282
Based on how it's calculated, particle_locationNow is a total position, not global.
globalPos is also called in a misleading way, it is an index of the current cell in total coordinates, not global.
However, this fix seems valid. particle_locationNow[d] is in total coordinates and globalOffset[d] * cellSize[d] is a shift of global origin from total origin, so the difference is particle position in the global domain - which seems what we need there.
Imprecise naming as I pointed above made it not very clear, so perhaps could also be improved as a second commit to this PR?
My semantic understanding of particle_locationNow, based on the way it's computed, was that it is the global poosition in the entire simulation, i.e. in-cell + in domain offset + domain offset.
Changing nomenclature is, of course, possible. But maybe it's better done in a separate PR, to keep this one focused on the actual problem. I expect some changes to this PR, since in discussion with @PrometheusPi a potential problem came up:
The way the shift is calculated above it may lead to a "jumping" radiation window due to globalOffset sliding over GPUs. Unfortunately I have no set-up that could be used to emphasize and evaluate this potential problem. Skipping the y-axis for the window function was one suggestion, but it would needlessly impact non-movin-window simulations.
that it is the global poosition in the entire simulation, i.e. in-cell + in domain offset + domain offset.
This way describes calculating both total and global position. The difference is whether the domain offset in question is local domain to global domain start or local domain to total domain start. In this case I think it's the latter, so it is a total position at the end.
Sure, the naming change is optional and your fix as-is seems fine. I would say the naming was part of the problem since the chosen naming hid the issue. But don't insist, and if there are plans for further change maybe it's better to do then.
@Anton-Le please rebase against the dev branch to solve the CI issue.
I intend to keep this in draft state for the moment to see whether the undulator set-up will suffice as a test.
