Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NumPy 2 compatibility for tests and tools #1315

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

bnavigator
Copy link
Contributor

For NumPy 2 compatibility, besides #1303 (yes it makes a difference because of the changed API), the tests and tools need to be adjusted as well.

@larsoner
Copy link
Collaborator

larsoner commented Sep 2, 2024

@bnavigator let me know when I should take a look here

@bnavigator
Copy link
Contributor Author

I don't think I have anything to add? The test failures seem unrelated.

The patch works fine for 4.8.2 here: https://build.opensuse.org/request/show/1198163

@larsoner
Copy link
Collaborator

larsoner commented Sep 3, 2024

@bnavigator I'd feel better about merging if we could get the tests green. Okay for me to push some commits to your branch to get there?

@bnavigator
Copy link
Contributor Author

Sure

@larsoner
Copy link
Collaborator

Also adding a fix for enthought/apptools#351

Copy link
Collaborator

@larsoner larsoner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay all green! @prabhuramachandran ready for review/merge from my end. @bnavigator feel free to double-check that I didn't break anything for you.

pref_file = pkg_resources.resource_stream(pkg, pref)

pref_file = importlib.resources.files(pkg).joinpath(pref)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg_resources is deprecated, switch to importlib_resources

Comment on lines +682 to +685
# Weirdness on NumPy 2.1 and vtk >= 9.3 that this does not show up as
# an option and creates problems
if klass.__name__ == "vtkPoints" and m == "DataType" and sys.platform == "win32":
d["int32"] = vtk.VTK_ID_TYPE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This took a really long time for me to find. No idea why but on NumPy 2.1 and VTK 9.3+ vtkPoints get created with datatype 12 i.e. VTK_ID_TYPE and it wasn't showing up in the RevPrefixMap for Points, causing a cryptic error:

Traceback
____________ TestMTriangularMeshSource.test_reset_changes_pipeline ____________
tvtk\tvtk_base.py:235: in validate
    n = len(value)
E   TypeError: object of type 'int' has no len()

During handling of the above exception, another exception occurred:
mayavi\tests\test_mlab_source.py:991: in test_reset_changes_pipeline
    obj.mlab_source.reset(x=x, y=y, z=z, triangles=triangles, scalars=z)
mayavi\tools\sources.py:832: in reset
    pd.trait_set(points=points)
C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\traits\has_traits.py:1509: in trait_set
    setattr(self, name, value)
tvtk_classes\point_set.py:122: in _set_points
    ???
tvtk_classes\point_set.py:120: in _get_points
    ???
tvtk_classes\tvtk_helper.py:65: in wrap_vtk
    ???
tvtk_classes\points.py:45: in __init__
    ???
tvtk\tvtk_base.py:432: in __init__
    self.update_traits()
tvtk\tvtk_base.py:585: in update_traits
    setattr(self, name, val)
tvtk\tvtk_base.py:247: in validate
    self.error(object, name, value)
C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\traits\base_trait_handler.py:74: in error
    raise TraitError(
E   traits.trait_errors.TraitError: The 'data_type' trait of a Points instance must be 'bit' or 'char' or 'double' or 'float' or 'int' or 'long' or 'short' or 'unsigned_char' or 'unsigned_int' or 'unsigned_long' or 'unsigned_short' (or any unique prefix) or 1 or 10 or 11 or 2 or 3 or 4 or 5 or 6 or 7 or 8 or 9, but a value of 12 <class 'int'> was specified.

This fixes it. Not 100% sure it's correct but tests at least pass 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants