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

Implement multifab wrapper functionality from WarpX #370

Merged
merged 27 commits into from
Oct 10, 2024

Conversation

dpgrote
Copy link
Contributor

@dpgrote dpgrote commented Sep 27, 2024

This adds the code from the WarpX fields.py that allows global indexing of MultiFabs. This is done by adding __getitem__ and __setitem__ methods for the MultiFab object.

Note that this incorporates the changes from WarpX PR #4370 that had not been merged in.

This currently only does the get/set without including the ghost cells. What is needed is a way of specifying inclusion of the ghost cells. Perhaps this?

mf = MultiFab()
mf[:,5,6]  # without ghost cells
mf[:,5,6] = 2.  # without ghost cells
mf.with_ghosts([slice(None),5,6])  # with ghost cells (in the first dimension)
mf.with_ghosts([slice(None),5,6], 2.)  # with ghost cells

Maybe this, using imaginary numbers to refer to ghost cells?

mf[-1j,5,6]  # the first ghost cell on the lower boundary
mf[+1j,5,6]  # the first ghost cell on the upper boundary
mf[(),5,6]  # the full first dimension including ghosts
mf[:,5,6]  # the first dimension without ghosts

I prefer this method.

@dpgrote dpgrote added the enhancement New feature or request label Sep 27, 2024
@ax3l
Copy link
Member

ax3l commented Sep 30, 2024

Hi @dpgrote,

That is fantastic, thanks a lot for porting this!

I think in symmetry with AMReX functions like Box::grow(), the syntax .with_ghosts is clearer. Could also be .grow if we want, but either way is great.

We could even add default arguments to =nGrowVect() for the function so it can be called .with_ghosts() and get the ghosts.

@ax3l
Copy link
Member

ax3l commented Sep 30, 2024

@dpgrote This is awesome. To add CI and show usage examples, can you please add a few new examples in tests/?

This then makes it very easy to add add a new tab in fields, using the snippet from the test then in the user-facing doc page:

src/amrex/extensions/MultiFab.py Outdated Show resolved Hide resolved
src/amrex/extensions/MultiFab.py Outdated Show resolved Hide resolved
src/amrex/extensions/MultiFab.py Outdated Show resolved Hide resolved
src/amrex/extensions/MultiFab.py Outdated Show resolved Hide resolved
src/amrex/extensions/MultiFab.py Outdated Show resolved Hide resolved
@dpgrote
Copy link
Contributor Author

dpgrote commented Sep 30, 2024

I think in symmetry with AMReX functions like Box::grow(), the syntax .with_ghosts is clearer. Could also be .grow if we want, but either way is great.

We could even add default arguments to =nGrowVect() for the function so it can be called .with_ghosts() and get the ghosts.

How about this, add the method mf.alias_with_ghosts() that returns an alias of the MF with the boxes grown by the number of ghost cells. Then the __getitem__ and __setitem__ can be use directly.

Note that there is a subtlety in __setitem__ regarding ghosts. With include_ghosts False, only valid cells are set. When True, the ghost cells of the individual blocks are set as well (as if FillBoundary had been called). Perhaps the internal ghost cells should always be set?

from .Iterator import next

try:
Copy link
Member

Choose a reason for hiding this comment

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

We need to put this into a function that has access to amr, so we only attempt an import if:

if amr.Config.have_mpi:

Otherwise, this can cause issues in scenarios, e.g., running serially on Cray machines.

try:
from mpi4py import MPI as mpi

comm_world = mpi.COMM_WORLD
Copy link
Member

Choose a reason for hiding this comment

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

While we have access to amr, we can replace mpi.COMM_WORLD with amr.ParallelDescriptor.Communicator() later on.

I did not expose that element yet, but we already created an mpi4py compatible MPI_Communicator for the MPMD logic in pyAMReX. This can be a follow-up PR.

@dpgrote
Copy link
Contributor Author

dpgrote commented Oct 3, 2024

The more I think about this (and work on implementing something), the more I like the idea of using imaginary numbers for the guard cells. The idea of using negative numbers for the lower ghost cells breaks the standard python usage, since for every sequence type, negative indices are always relative to the end of the sequence. It is asymmetric since there then no nice way of referring to the upper ghost cells. The imaginary indices are a very concise and unambiguous way is specifying the ghosts, both lower and upper. I'll implement this and provide examples to show what it's like.

@ax3l
Copy link
Member

ax3l commented Oct 3, 2024

So to get a plane with ghost cells, something like this? :)

mf[-2j:+2j, 0, 0]  # include 2 ghost cells in the first plane of the global domain

or is that

mf[-2j:-1+3j, 0, 0]  # include 2 ghost cells in the first plane of the global domain

@dpgrote
Copy link
Contributor Author

dpgrote commented Oct 4, 2024

So to get a plane with ghost cells, something like this? :)

mf[-2j:+2j, 0, 0]  # include 2 ghost cells in the first plane of the global domain

or is that

mf[-2j:-1+3j, 0, 0]  # include 2 ghost cells in the first plane of the global domain

It would be

mf[-2j:0, 0, 0]

The -2j refers to the 2nd ghost cell. 0 refers to the first valid cell, remembering python slice range that goes up to but does not include the end of the slice range.

If you want all of the cells, the valid cells plus the two guard cells on each side, then it would be

mf[-2j:3j,0,0]

One thing that is missing is a clean way to specify and open ended range on one side including the ghost cells.

Copy link
Member

@ax3l ax3l 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 awesome and impressive, thank you! 🚀 ✨

@ax3l ax3l merged commit 9c51d04 into AMReX-Codes:development Oct 10, 2024
17 of 18 checks passed
@dpgrote dpgrote deleted the implement_multifab_wrapper branch October 10, 2024 18:49
atmyers pushed a commit that referenced this pull request Oct 17, 2024
This fixes some comments from PR #370
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants