MultiFab::deepCopy()

[ax3l]

Summary

Create a deep copy of this MultiFab, using the same Arena and factory.

Additional background

See AMReX-Codes/pyamrex#282

Checklist

The proposed changes:

• fix a bug or incorrect behavior in AMReX
• add new capabilities to AMReX
• changes answers in the test suite to more than roundoff level
• are likely to significantly affect the results of downstream AMReX users
• include documentation in the code and/or rst files, if appropriate

[WeiqunZhang]

We should use a different name, because MultiFab::copy will hide FabArray::copy and therefore is a breaking change. Also we already have MultiFab::Copy. So it can be quite confusing. (We even deprecated FabArray::copy to avoid confusion.

[ax3l]

We should use a different name, because MultiFab::copy will hide FabArray::copy and therefore is a breaking change.

I am not sure it does - they have different arguments in their signatures...?

Also we already have MultiFab::Copy. So it can be quite confusing. (We even deprecated FabArray::copy to avoid confusion.

I added the lowercase variant similar to the uppercase (Add - static) and lowercase (add - method) naming so far. Primarily, I added it because in Python containers like NumPy nd arrays, .copy() is the canonical way to do a by-value copy. AMReX-Codes/pyamrex#282

Should we add this extra copy() method overload to FabArray instead?

[WeiqunZhang]

That's how C++ works. https://isocpp.org/wiki/faq/strange-inheritance#hiding-rule (We could work around it by using FabArray<FArrayBox>::copy. But function overloading can be confusing to users.)

[ax3l]

Haha, ah yes I sometimes forget that fun detail :D
We have do that in ImpactX elements, too.

Python has no overloading at all... unless we are in pybind11 😆

We could work around it by using FabArray::copy. But function overloading can be confusing to users.

Sounds safe to me, since we do not have default parameters on the existing copies...?

[WeiqunZhang]

What makes me uneasy is its implication. We have so far repeatedly resisted the urge of making it too easy to create a deep copied object. The copy ctor is deleted. We have a ctor for creating an alias MultiFab, which takes a parameter amrex::MakeType, but only make_alias is allowed. The new copy function is just like a copy ctor. One might be attempted to write the following correct but inefficient code.

MultiFab a = b.copy();
a = 0;

So let's have some time to think about this. Maybe we should make the function name more obvious like createDeepCopy. Maybe we could update the ctor to allow amrex::MultiFab a(amrex::make_deep_copy, b);.

[ax3l]

Yes, I very much sympathize where this comes from. Here, I think there is a reasonable difference: in C++, the copy constructor and copy assignment are syntactically very reasonable to delete for large containers such as MultiFab, because they can be implicitly be called and lead to surprising resource costs.

This is essentially the same reason why Python does by-ref for everything by default - no implicit copies of containers ever.

Contrary to copy constructors and copy assignment operators, adding a .copy() method is explicit and a clear resource allocation when it is needed.

One might be attempted to write the following correct but inefficient code.

Maybe I miss a point. Yes, we should definitely expose make_alike the same way so we create an exact shallow copy without the values being copied (i.e., for your example doing immediately an a = 0;)

MultiFab a = b.copy();
a *= -1;

MultiFab a = b.make_alike();  // or b.copy(/* deep= */ false)
a = 0;

Let's chat about this more. I think that is a reasonable syntax and explicit once a copy is actually needed. It also uses canonical naming for deep copies, so we definitely would want this downstream in pyAMReX.

• https://numpy.org/doc/stable/reference/generated/numpy.copy.html - deep
• https://docs.cupy.dev/en/stable/reference/generated/cupy.copy.html - deep
• https://docs.xarray.dev/en/latest/generated/xarray.DataArray.copy.html - deep=True default
• https://pytorch.org/docs/stable/generated/torch.clone.html (close xD) - deep

My feeling is that making it equally explicit and not more verbose/complicated to create deep copies is desirable.

[ax3l]

To avoid confusion with FabArray<FAB>::copy (which could be called copyFrom() and are deprecated for the use of ParallelCopy instead) we name this: deepCopy().

In pyAMReX, because FabArray<FAB>::copy is deprecated, we will not expose it to Python and keep naming this .copy() as is canonical in Python.