Expose num_threads when reading in meshes from XDMFFile and VTKHDF

[MariusCausemann]

Addresses #4521

[jorgensd]

@MariusCausemann can you add some of the notes with respect to your findings of nanobind lookup?

[MariusCausemann]

Sure! This is what claude said about it, shortened by me:

"_cpp.mesh.create_mesh has two overloads, and the mixed-topology one is registered first. nanobind tries overloads in the order they were registered. For each one it converts the arguments from left to right and stops at the first argument that doesn't fit. The old read_mesh passed (comm, cells, cmap, x, ...) positionally, with cells as a 2D array of shape (662,243, 4):

• nanobind tried the 1st overload.
• comm converted.
• cells also converted. The vector converter accepts any sequence, and a 2D NumPy array is a sequence of rows. Each row is a contiguous 1D int64 array, so it passes the per-element check. nanobind built 662,243 row objects.
• cmap failed, because a single CoordinateElement is not a sequence. All that work was thrown away.
• nanobind then tried the 2nd overload. That conversion is cheap, and the call went into C++.

nanobind matches keyword names before it converts anything. The 1st overload has no parameter called element (its elements), so nanobind rejects it straight away."

On my example mesh, using keywords reduced read_mesh timing from 0.781s to 0.507s.
I found this quite surprising, not sure if there is a general fix. It might be a good idea to use less overloading, and give each function (here the mixed dim read_mesh) its own name.

edit: On a larger mesh, the nanobind issue alone halves the read_mesh time, the wasted work is proportional to the number of cells.

[schnellerhase]

Yikes, nice find! Does this mean this is now faster because of differing elements vs. element arg name 🫣?

If so I don't think this is the right fix. We should remove either one of the overloads to unify to a single create_mesh or give unique names. Secondly which will probably remove most of the runtime difference we should mark the input arrays with noconvert() to disallow conversion to even try.

[jorgensd]

Add dtype to fem.form.

[MariusCausemann]

done

[jorgensd]

Yikes, nice find! Does this mean this is now faster because of differing elements vs. element arg name 🫣?

If so I don't think this is the right fix. We should remove either one of the overloads to unify to a single create_mesh or give unique names. Secondly which will probably remove most of the runtime difference we should mark the input arrays with noconvert() to disallow conversion to even try.

I think using keyword arguments as done in this PR is fine. It of course doesn't fix the issue on a deeper level.

[MariusCausemann]

I added a fix on the nanobind level now, allowing only list or tuple for the cells argument in the mixed dim create_mesh call. This fixes the issue, what do you think @jorgensd and @schnellerhase ?
I tried noconvert(), but it nanobind still turns the 2D array into a tuple of rows, so it did not help.

I also had claude look through the whole dolfinx repo for similar nanobind issues, but it claims everything else is fine.

[schnellerhase]

I tried noconvert(), but it nanobind still turns the 2D array into a tuple of rows, so it did not help.

You are right, that does not solve the overload standalone - the array also is a sequence so it is no conversion initially.

The new code path does not look preferable to me: a lot of special case handling and explicit casting. Revert c4dc8c6, file an issue and can be fixed else where as @jorgensd suggested?

[schnellerhase]

Simpler with std::vector<std::span<const std::int64_t>> cells = vec_of_spans(cells_nb);.

[MariusCausemann]

I have reverted the last commit, and opened a new issue for the nanobind overload #4542