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

Merged
merged 22 commits into from
Dec 17, 2024

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 🤷

@opoplawski
Copy link

Ping? Numpy 2.0 has hit fedora rawhide so it would be good to get mayavi updated.

Copy link
Member

@prabhuramachandran prabhuramachandran left a comment

Choose a reason for hiding this comment

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

LGTM, so sorry I have taken forever to merge this.

@prabhuramachandran prabhuramachandran merged commit 40c6010 into enthought:master Dec 17, 2024
14 checks passed
@prabhuramachandran
Copy link
Member

Thanks @larsoner for all the hard work, this is now merged. Please let me know if there are any other issues.

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.

4 participants