Change writeable flag after moving array from C++

[jorgensd]

Resolves #2625.
Not the prettiest of fixes, but I cannot see a nice workaround for this.

[garth-wells]

See pybind/pybind11#1466.

[jorgensd]

See pybind/pybind11#1466.

I cannot get that to work with a py::array_t structure.
One could use the C++ equivalent of the current solution, suggested in:
pybind/pybind11#481 (comment)
It seems like the buffer solution never made its way up to array_t:
pybind/pybind11#338 (comment)

[jorgensd]

See pybind/pybind11#1466.

I cannot get that to work with a py::array_t structure. One could use the C++ equivalent of the current solution, suggested in: pybind/pybind11#481 (comment) It seems like the buffer solution never made its way up to array_t: pybind/pybind11#338 (comment)

@michalhabera suggested the new approach, which is influenced by the C++ assign flag comment

[garth-wells]

This should be wrapped up in a helper function, otherwise we'll have low-level code everywhere.

[michalhabera]

This should be wrapped up in a helper function, otherwise we'll have low-level code everywhere.

@jorgensd I think Garth wrote wrappers/array.h for this purpose. My suggested approach might be still flawed wrt. lifetime management, py::cast(self) passed as a base also made lifetime of returned numpy array coupled with the MeshTags cpp object.

[jorgensd]

This should be wrapped up in a helper function, otherwise we'll have low-level code everywhere.

@jorgensd I think Garth wrote wrappers/array.h for this purpose. My suggested approach might be still flawed wrt. lifetime management, py::cast(self) passed as a base also made lifetime of returned numpy array coupled with the MeshTags cpp object.

How about:

        py::array_t<T> array(self.values().size(), self.values().data(),
                                 py::cast(self));
            reinterpret_cast<py::detail::PyArray_Proxy*>(array.ptr())->flags
                &= ~py::detail::npy_api::NPY_ARRAY_WRITEABLE_;
            return array;

I guess we could add this to the as_pyarray function. Just need to deduce if the arguments should be readonly or not

[garth-wells]

I think there is a mis-understanding of def_property_readonly. One should be able to change the values of a MeshTags object.

def_property_readonly means something is accessible as a property, but that there is no setter. I.e., one can access the mesh tag data and change the values, but one cannot associate a new array with the MeshTags object.

[jorgensd]

Not if you look at the C++ signature, which returns a const span https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/mesh/MeshTags.h#L98

[garth-wells]

Not if you look at the C++ signature, which returns a const span https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/mesh/MeshTags.h#L98

For the indices yes, but not for the values. The PR prevents users from changing the values.

Python doesn't generally have the concept of const, despite NumPy arrays having a 'writeable' flag. The philosophy is to trust users. What is important here is that that the owndata flag is correctly set since changing size would cause a corruption of the MeshTags object.

[jorgensd]

Not if you look at the C++ signature, which returns a const span https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/mesh/MeshTags.h#L98

For the indices yes, but not for the values. The PR prevents users from changing the values.

Python doesn't generally have the concept of const, despite NumPy arrays having a 'writeable' flag. The philosophy is to trust users. What is important here is that that the owndata flag is correctly set since changing size would cause a corruption of the MeshTags object.

The values are const span: https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/mesh/MeshTags.h#L101
and with making the modifiable, one makes the form domain Construction unclear, as that is done with the meshtags at initialization.

[garth-wells]

Not if you look at the C++ signature, which returns a const span https://github.com/FEniCS/dolfinx/blob/main/cpp/dolfinx/mesh/MeshTags.h#L98

Indeed, my mistake.

Still, I don't think it's unreasonable to be able change the values from Python, even if it risks breaking something else.

[jorgensd]

Im in favor of avoiding alot of unexpected behavior with the interaction with dolfinx::fem::Form, which has major assumptions on the const-ness of integration entities specified in MeshTags. As I mentioned in an earlier commit, we could also force this purely on the Python side (inside the property functions).

[jhale]

This should really be fixed at the pybind11 level.

Having said that, I fall on the side that the numpy array should be set as non-writeable, reflecting the underlying const specification in the C++ span. I don't have a strong view on whether the flag should be set at the C++/pybind11 level, or in the pure Python wrappers.

[jorgensd]

I agree with that, and I am happy with setting it in the pybinding, as done in this PR currently.

[garth-wells]

I'm not too fussed either way. In any case, a helper function should be added in array.h rather than adding the low-level code to each wrapper function.

Some lifetime/reference counting tests are need too since we're not 100% sure what is happing behind the scenes with pybind11 and what this PR does.

[garth-wells]

Note: nanobind handles const and NumPy arrays transparently.

[garth-wells]

This will be addressed when we switch to nanobind, see #2820.