Function to get particle cell index.

[archermarx]

Close #4977

This PR adds a function to get a particle's cell index to ParticleUtils.H, and uses it in both ParticleUtils.cpp and TemperatureFunctor.cpp.

[ax3l]

Thanks for the PR, @archermarx!
We just mitigated a CI issue, can you rebase against the latest development and force push (or merge development)? Thanks!

[archermarx]

yes, done!

[ax3l]

That looks splendid, thank you so much for this!

[archermarx]

Double checking, I had to cast a few parameters to void in order to avoid unused variable warnings due to y and/or z positions not being needed in 1D and 2D simulations. Is this ok, or is there a cleaner way to do this?

[roelof-groenewald]

Thanks for this cleanup! Just a couple small comments.

[archermarx]

Ok, I added an overload for the function when called on a SuperParticleType and incorporated it into TemperatureFunctor.cpp

[ax3l]

CI is fixed for good now. Once adding the update from @roelof-groenewald , please rebase once more @archermarx .

[archermarx]

Ok, made the requested changes and rebased

[roelof-groenewald]

I see most (maybe all) of the examples that involve sorting the particles per cell are still failing. Looking closer, I see that the current development function here calculates the cell indices with

ii = (p.pos(0) - plo[0]) * dxi[0] - lo.x

whereas the AMReX function uses

ii = (p.pos(0) - plo[0]) * dxi[0] + cbx.smallend[0]

So the difference is that the development version calculates a cell index starting from 0 for each tile whereas the AMReX function calculates a global cell index for the entire domain. It's not clear to me why this breaks the code though since the binning should only care about which particles have the same cell index, not about what the value of the indices are.

[roelof-groenewald]

I'll look more into the current reason for the test failures but I also wanted to point out that the new utility function can be used in https://github.com/ECP-WarpX/WarpX/blob/development/Source/Diagnostics/ComputeDiagFunctors/ParticleReductionFunctor.cpp in a similar way as in the temperature diagnostic.

[archermarx]

OK, I can add it in there too

[roelof-groenewald]

Looks like collisionXZ and collisionXYZ tests are still failing, but all other CI tests pass.

Great! Those tests have been problematic for a while now (see #5132). After that PR is merged you can rebase and they should pass.

[archermarx]

Oh great, that's good to know!

[roelof-groenewald]

I would be okay to merge this since it already cleans up the code quite a bit, but we should be able to replace

    AMREX_GPU_HOST_DEVICE AMREX_INLINE
    amrex::IntVect getParticleCellIndex (const WarpXParticleContainer::SuperParticleType& p,
                                         amrex::GpuArray<amrex::Real,AMREX_SPACEDIM> const& plo,
                                         amrex::GpuArray<amrex::Real,AMREX_SPACEDIM> const& dxi) {
        return getParticleCellIndex(p.pos(0), p.pos(1), p.pos(2), plo, dxi);
    }

with something like

    AMREX_GPU_HOST_DEVICE AMREX_INLINE
    amrex::IntVect getParticleCellIndex (const WarpXParticleContainer::SuperParticleType& p,
                                         amrex::GpuArray<amrex::Real,AMREX_SPACEDIM> const& plo,
                                         amrex::GpuArray<amrex::Real,AMREX_SPACEDIM> const& dxi) {
        // create zero_low_bound_box
        return amrex::getParticleCell(p, plo, dxi, zero_low_bound_box);
    }

where zero_low_bound_box is an amrex::Box that has the property zero_low_bound_box.smallEnd() == {0, 0, 0}. This would allow us to directly use the AMReX function and not need all the extra logic in WarpX repeated. @ax3l could you help us out with how to create zero_low_bound_box?
Or alternatively, we can maybe add an overload to AMReX for amrex::getParticleCell that does not take the 4th argument and in that case does not shift the cell index based on the global box index.

EDIT: On second thought, I like that second option. I'll go ahead and submit a PR to AMReX.

[archermarx]

That sounds good, and would let us eliminate a lot of code. For the version that takes positions instead of a particle type, is there a way we can delegate to getParticleCell as well? That version is only called in the WarpXParIter loop in TemperatureFunctor.cpp. Can we create a WarpxParticleType from the elements of that iterator?

[archermarx]

I just pushed a version that uses your overload of getParticleCell. Let me know what you think and if you think we can further simplify

[archermarx]

Ok, looks like that got merged into AMReX. Once we update AMReX for the week, this should work, I think

[roelof-groenewald]

@archermarx would you mind rebasing on development now that the AMReX version has been updated?

[archermarx]

Yes, will do in a moment

…

On Tue, Aug 20, 2024, 5:22 PM Roelof Groenewald ***@***.***> wrote: @archermarx <https://github.com/archermarx> would you mind rebasing on development now that the AMReX version has been updated? — Reply to this email directly, view it on GitHub <#5118 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOP3QGXQETHMM6DHSAVWLHDZSOXQ7AVCNFSM6AAAAABMFCQT66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJZG44DQNRSGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[archermarx]

To fix some further redundancy, I have eliminated the new function entirely and just call the new overload of amrex::GetParticleCell. To do that, I had to make a small modification to TemperatureFunctor.cpp to grab a ParticleType instead of using the GetPosition API. Let me know if there's a cleaner way to do this than the way I'm doing it.

[archermarx]

CI should pass now. I tried replacing the particle cell finding in ParticleUtils.cpp with a use of amrex::getParticleCell but CI failed for some reason. Not sure why.

[roelof-groenewald]

Thanks for this PR! I think PRs that remove this many lines of code without altering functionality (or obscuring it) are really great!

[ax3l]

Ah, there is another branch, m_do_average, below that can use the same functionality.

[ax3l]

Follow-up in #5557