Issues in pyvista with upcoming VTK changes

[berkgeveci]

Hello everyone.
I am one of the VTK developers. We are working on some changes to VTK that are causing issues with pyvista and we could use some help from the community. The changes, which can be seen here, add support for the creation of Python properties in the VTK Python wrappers. In short, the wrappers will create a property called point_data when they see a method called GetPointData(). This is overall working well. However, we ran into issues in pyvista. This is because pyvista dataset classes are multiply inherited and after this change, the new VTK properties are taking precedence over pyvista properties. For example:

class PolyData(_vtk.vtkPolyData, _PointSet, PolyDataFilters):

In this cases, the point_data property defined in vtkPolyData hides the point_data property defined in _PointSet. I tried fixing this with:

class PolyData(_PointSet, _vtk.vtkPolyData , PolyDataFilters):

however, there are several places where super() is called and is assumed to return the VTK type, not the pyvista mixin. I quickly got lost in this class hierarchy trying to fix this.

I can think of two ways of dealing with this:

1. Change the order of superclasses as I started and fix all issues with using the second superclass where appropriate,
2. Switch to using the VTK defined properties but replace the classes returned with using the @overload functionality (which allows substituting a subclass of, for example, vtkPointData with your subclass of vtkPointData written in Python.

Option (1) is probably the most straightforward. Option (2) may be cleaner. We are going to option (2) in VTK's Python modules to create more pythonic dataset classes.

Thanks.

[banesullivan]

1. Change the order of superclasses as I started and fix all issues with using the second superclass where appropriate,

This seems like an appropriate solution to me, but I would need to take time to give it a try to see what issues this may provide.

2. Switch to using the VTK defined properties

It's unclear to me what you mean. Would you please elaborate on this? Are you suggesting that we leverage VTK's soon-to-come point_data attribute instead of PyVista's current wrapping of PointData under the point_data attribute?

We are going to option (2) in VTK's Python modules to create more pythonic dataset classes.

I'm interpreting from this statement that VTK would soon to provide its own new Python wrappings, is that correct?

[berkgeveci]

I found a much cleaner solution I think!

Here is an example:

class dummy(object):
    @property
    def point_data(self):
        print('in dummy')
        return 12
    @point_data.setter
    def point_data(self, foo):
        self.pd = 12

class foo(vtk.vtkPolyData, dummy):
    point_data = dummy.point_data

So the easiest thing to do is to manually overwrite the few properties that will come from VTK.

[berkgeveci]

I'm interpreting from this statement that VTK would soon to provide its own new Python wrappings, is that correct?

I will soon write a post on the VTK Discourse about this. We are improving the Python wrapper to automatically generate properties. We are also going to improve (or rewrite) the dataset_adapter module to be more Python/Numpy friendly. We will likely use the @override decorator with the new Python datasets so if one imports the adapter module, they will get the new API with all data object instances.

[banesullivan]

I see, this sounds interesting and look forward to the discourse post outlining it! Please try to ping us here with a link if you can.

Ultimately, I hope we can keep synergy between these upstream efforts in VTK's dataset_adapter module and PyVista to provide an amazing suite of tools for all 3D visualization Python users!

[berkgeveci]

Sure, will do. I would like to have a very tight integration between the improved interface and PyVista & Vedo.

[darikg]

The changes, which can be seen here, add support for the creation of Python properties in the VTK Python wrappers. In short, the wrappers will create a property called point_data when they see a method called GetPointData()

that sounds really nice. Will the properties also be settable through the constructor? E.g. pd = vtkPolyData(points=vtkPoints(...))

[berkgeveci]

that sounds really nice. Will the properties also be settable through the constructor? E.g. pd = vtkPolyData(points=vtkPoints(...))

Yes. See https://gitlab.kitware.com/berkgeveci/pythonic-vtk for a prototype. Note that this is a sandbox to explore ideas not the actual implementation. If you look at datamodel.py and filters.py, you can see some of the ideas I have explored.

[berkgeveci]

Hi all,
So, I worked around the issue I mentioned above by essentially doing something like this:

class UnstructuredGrid(_vtk.vtkUnstructuredGrid, PointGrid, UnstructuredGridFilters):

    point_data = PointGrid.point_data
    cell_data = PointGrid.cell_data
    field_data = PointGrid.field_data

This has to be done only in a few places and is fairly clean. However, I ran into a bigger issue. As part of the wrapper changes, we now have the constructor of the VTK type parse keyword arguments and initialize properties. Well, that works great except in pyvista.Property. What happens is that the generated code goes over all of the keyword arguments with default values in Property's __init__() and calls the class' property method. So, it ends up calling, for example, the metallic property in pyvista.Property with None, which chokes on the range checking code. On the other hand, the __init__() method nicely filters these values and turns them into appropriate default values. Not something the VTK code has the luxury of doing. I am at a loss for how to fix this. It will likely require a major refactoring of how these default values are handled. Advice would be much appreciated.

[berkgeveci]

Hi all,

I made some progress in fixing issues but there is more to go. Here is a very much work in progress diff:

diff.txt

There are two sets of issues that I have encountered so far:

1. VTK's new properties taking precedence over the PyVista ones. This requires either changing the order of inheritance (which seems to be tricky to me) or bringing back the PyVista properties explicitly. (see the diff for some examples)
2. Keyword arguments in some classes (so far, only Property type classes as far as I can tell) leading to VTK initializing properties and leading to errors due to incompatible default values. I think that the way to go around this is to restructure the code so that the constructors take **kwargs and do the proper initialization in the body of the __init__() method. Also, any code that passes None default values to the constructor needs to be changed not to do that.

As a reminder, the work-in-progress merge request is here:
https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10820

We aim to merge this in the next couple of weeks so some help addressing the PyVista issues sooner rather than later would be very appreciated.

PS: I verified that these changes do not effect vedo.

Thanks.

[banesullivan]

I verified that these changes do not effect vedo.

That's good news, though I still have concerns as Vedo does indeed use inheritence (but perhaps there are no name conflictions?) The following are links I was not allowed to include in my GitLab comment: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/10820#note_1471386

There seems to be a number of places where Vedo directly inherits from VTK classes:

• vtkPropAssembly
• vtkAssembly
• vtkActor2D
• vtkInteractorStyleUser
• vtkFlagpoleLabel
• vtkContourWidget
• vtkSliderWidget
• and other places

[banesullivan]

Here is a very much work in progress diff

Would you be open to creating a draft pull request of these changes so that the community can engage with you directly on these modifications?

[berkgeveci]

Would you be open to creating a draft pull request of these changes so that the community can engage with you directly on these modifications?

I would be too embarrassed to make a merge request out of this :) Let me clean it up a bit.

I'll look at those vedo classes carefully. Thanks.

[banesullivan]

I would be too embarrassed to make a merge request out of this :) Let me clean it up a bit.

We're a casual bunch here and no judgement will be issued :) My thought is that we'd like to treat this as a prototype and iterate with you to come up with the final approach. FYI, we squash all commits in PRs upon merge, so it won't be in the history

[berkgeveci]

Here it is:
#5500

[berkgeveci]

Hi folks. Any progress on this topic? We made great progress on the properties interface and we are preparing to merge. It will be another week probably.

[banesullivan]

Excited about these upstream improvements! I'll try to take some time this weekend to make sure pyvista is compatible

[banesullivan]

I just pushed up some draft work in #5650