vtkCell wrappings slow down

[banesullivan]

@bjlittle is experiencing some slow downs from the new wrapping of vtkCell in 0.38+

performance bottleneck that I was hitting when using pyvista 0.38+. Basically it's a result of the new wrapping of vtkCell and calls to mesh.cell[cid] instead of mesh.get_cell(cid)... Which makes a massive difference, particularly for large/huge meshes.

We should look into this

[MatthewFlamm]

We should strongly discourage use of mesh.cell[cid]. Basically, the only useful way it can be used is for cell in mesh.cell: But even this is not the best.

[bjlittle]

Thanks for raising this @banesullivan 🍻

Basically, prior to v0.38.x I was using the pyvista.core.dataset.cell_points_ids and pyvista.core.dataset.cell_points methods, both of which are behaved well for large-ish meshes (by large-ish I mean meshes with ~600K-1M cells)

The introduction of #3715 for v0.38.x causes a major chronic performance hit. I mean... seriously bad.

The good news is that after understanding what was happening and the change that was made, I was able to easily circumvent this issue by using pyvista.core.dataset.get_cell instead. Happy days!

I guess the discussion point here is whether out-the-box it's okay for pyvista to default to this poor performance hit (for large meshes) and even advertise to users that they should adopt the cell[i].point_ids pattern in the deprecation warning?

Dunno. Tought call.

Would it not in general be better to use the get_cell(i).point_ids pattern instead? As it's just massively more efficient 🤔

This issue just seems avoidable. What do you guys think?

[bjlittle]

BTW as part of your infrastructure, do you guys perform automated benchmarking of pyvista, say using asv, to catch performance degradations/improvements ?

[MatthewFlamm]

I'm +1 to remove cell as I had the same observation about slowness in the original PR. Or, we could consider making it a generator instead to better discourage making it a list.

[banesullivan]

we could consider making it a generator instead to better discourage making it a list.

+1 for a generator

[akaszynski]

we could consider making it a generator instead to better discourage making it a list.

Agree as well. There were some limitations with this approach that we fixed by using a new API, but we never revisited the generator vs. list discussion.

Adding to v0.39 milestone...

[MatthewFlamm]

I noticed from #4013 that UnstructuredGrid.cells is named very similarly. We need to clarify this better in the documentation if it cannot be collapsed together.