fields.py relies on MultiFab global indexing

[dpgrote]

The global indexing capability for MultiFabs was moved to pyamrex. This PR deletes the now not needed code from WarpX, relying on the capability in pyamrex.

Note that the MultiFab wrapper is maintained since it is persistent across load balancing, while the underlying MultiFabs are deleted and regenerated.

This is an API breaking change since there are differences in how to access the ghost cells when using the global indexing.

[EZoni]

@dpgrote

The PR looks good to me in general. There is only a merge conflict to fix. Since this contains an API breaking change, personally I don't remember how we had decided to roll this out. @ax3l and @RemiLehe might want to comment on this.

[ax3l]

Note that the MultiFab wrapper is maintained since it is persistent across load balancing, while the underlying MultiFabs are deleted and regenerated.

I might need a recap why we want to keep the named wrappers (like ExWrapper) though. I thought we want to remove them, because we have to keep their class definitions in sync with all potential multifabs we register, and we might add more at runtime that then have no corresponding wrapper.

Wouldn't it be also just fine to query the fields directly from warpx.multifab again when we next access them (potentially after a load balance)? That way we could remove the extra wrappers.

[dpgrote]

Note that the MultiFab wrapper is maintained since it is persistent across load balancing, while the underlying MultiFabs are deleted and regenerated.

I might need a recap why we want to keep the named wrappers (like ExWrapper) though. I thought we want to remove them, because we have to keep their class definitions in sync with all potential multifabs we register, and we might add more at runtime that then have no corresponding wrapper.

Wouldn't it be also just fine to query the fields directly from warpx.multifab again when we next access them (potentially after a load balance)? That way we could remove the extra wrappers.

Having the named wrappers is a convenience for the users, providing a list of the available MultiFabs in one place without having to search through the code. This also provides a nice way of fetching the MultiFabs without having to know the format of the MultiFab names in the registry. This is not essential - for instance a user could list the keys of the MF registry to get the names. But that is only available at runtime.

Yes, a user could always fetch the MF whenever they want to use it, but this puts on the onus on the user to do this. It would also put an onus on the user to keep track of when load balancing is done, which is likely to be problematic. With the wrapper, the user doesn't have to think about this and doesn't even have to be aware of the issue.

[EZoni]

Just one more question, to prepare for more discussions later on.

It's not clear to me if we reached consensus on the fact that this new syntax

rho = RhoFPWrapper()[:, :]    # exclude ghost cells
rho = RhoFPWrapper()[(), ()]  # include ghost cells

is more user friendly than the multifab syntax, and/or than the old syntax for the wrappers, which used an input parameter include_ghosts.

My understanding based on the discussion we just had during the developers meeting is that @jlvay, and possibly others, were concerned about keeping a syntax that is user-friendly (e.g., more Warp-like) and claimed that the new interface that relies on multifab isn't user-friendly enough.

What makes this new syntax for the old wrappers (example above) more user-friendly than the multifab syntax, given that we are advocating for not exposing the multifab syntax to the user because of the fact that it is not user-friendly enough (and because of the missing feature of guaranteeing a pointer to valid memory, which however could be probably integrated/added, wherever missing)?

It's possible that I got confused about which arguments and/or objections were raised during the meeting.

[roelof-groenewald]

@EZoni from my perspective, there's no problem with changing the notation used for indexing (especially since only access to guard cells are changed). Bringing the notation used with fields.py wrappers into alignment with what is used directly in pyamrex is a good thing. I just objected to removing the fields.py wrappers altogether, since that would require a significantly bigger update on our end than only changing how the indexing is done.

[roelof-groenewald]

@RevathiJambunathan and @clarkse have you guys had a chance to look over this PR yet?

[EZoni]

Before merging, please note that none of the GitHub Actions workflows ran on the last commit, not sure why. It may be worth amending the commit to re-trigger the CI checks.

[EZoni]

~On another note, two big PRs will be merged soon:~ ~- #3630~ ~- #5682~

~Since those PRs introduce new CI tests, I would recommend that we merge development here, after those PRs are merged, before resetting the checksums in this PR.~

[roelof-groenewald]

Since those PRs introduce new CI tests, I would recommend that we merge development here, after those PRs are merged, before resetting the checksums in this PR.

@EZoni, this PR does not reset any checksums, so I don't think it is necessary to wait for those other PRs. Or am I missing something?

[EZoni]

@roelof-groenewald

Sorry, I meant to post my message on #5820. Too many PRs opened in the browser...

[roelof-groenewald]

I think we should go ahead an merge this to avoid the need for @dpgrote to continually keep it from going stale. Any objections @RevathiJambunathan?

[clarkse]

I have no issue with the change in syntax. Looks good to me. An announcement after this gets merged would be nice so I know to go update my personal toolkits.

[roelof-groenewald]

@dpgrote sorry for the delay in merging this. I take it all interested parties have now had a chance to weigh in on this and everyone is satisfied that we can merge this change. I see there is now one conflict though. Could you please resolve that so we can merge the PR?