The two remaining test failures as of opening this PR come from ImageData.image_threshold(). And it has to do with this block in the filter:

Now, it would be easy to tweak that filter (probably by removing the NumPy version condition, depending on how our CI matrix looks on various OSes), but we seem to have this logic: "linux/mac doesn't threshold correctly on NumPy < 2.0", but now it also doesn't threshold on NumPy 2.0 (at least on my Linux). I have a suspicion that what's really going on is "it only works on Windows, NumPy < 2.0" which would likely have to do with the fact that "Windows NumPy < 2.0" is the only system where the default int size has 32 bits (this was fiiiinally fixed for NumPy 2.0).

So I want to take a closer look at this thresholding logic to see what's really going on here. I suspect it's Windows with NumPy < 2.0 that's somehow broken and needs to be special cased...

Ping @MatthewFlamm.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.09%. Comparing base (db82db5) to head (693f40e).

@@            Coverage Diff             @@
##             main    #6156      +/-   ##
==========================================
- Coverage   97.09%   97.09%   -0.01%     
==========================================
  Files         141      141              
  Lines       25939    25937       -2     
==========================================
- Hits        25185    25183       -2     
  Misses        754      754              

The two remaining test failures as of opening this PR come from ImageData.image_threshold(). And it has to do with this block in the filter:

pyvista/pyvista/core/filters/image_data.py

Lines 421 to 434 in b8c4dc4

	# For some systems and/or Numpy < 2.0, integer scalars won't threshold
	# correctly. Cast to float in these cases.
	has_int_dtype = np.issubdtype(
	array_dtype := self.active_scalars.dtype,
	int,
	) and array_dtype != np.dtype(np.uint8)
	cast_dtype = (
	has_int_dtype
	and int(np.__version__.split('.')[0]) < 2
	and platform.system() in ['Darwin', 'Linux']
	)
	if cast_dtype:
	self[scalars] = self[scalars].astype(float, casting='safe')

Now, it would be easy to tweak that filter (probably by removing the NumPy version condition, depending on how our CI matrix looks on various OSes), but we seem to have this logic: "linux/mac doesn't threshold correctly on NumPy < 2.0", but now it also doesn't threshold on NumPy 2.0 (at least on my Linux). I have a suspicion that what's really going on is "it only works on Windows, NumPy < 2.0" which would likely have to do with the fact that "Windows NumPy < 2.0" is the only system where the default int size has 32 bits (this was fiiiinally fixed for NumPy 2.0).

So I want to take a closer look at this thresholding logic to see what's really going on here. I suspect it's Windows with NumPy < 2.0 that's somehow broken and needs to be special cased...

See #6100.

The issue with image_threshold was initially identified as an issue with the docs build , i.e. linux. But the tests were added with that PR were also found to fail with some MacOS builds.

This may not be NumPy 2.0 specific. I think I added this condition int(np.__version__.split('.')[0]) < 2 because I saw that the NumPy 2.0 CI checks were passing. But in hindsight, I think these checks always passed. So, this condition can probably be removed.

EDIT: By "I think these checks always passed" , I mean that there is always green checkmark. So, the actual 2.0 tests may have still been failing for these builds, but the workflow was marked nevertheless marked as "succeeded"

Thanks for the tips @user27182.

This may not be NumPy 2.0 specific. I think I added this condition int(np.__version__.split('.')[0]) < 2 because I saw that the NumPy 2.0 CI checks were passing. But in hindsight, I think these checks always passed. So, this condition can probably be removed.

EDIT: By "I think these checks always passed" , I mean that there is always green checkmark. So, the actual 2.0 tests may have still been failing for these builds, but the workflow was marked nevertheless marked as "succeeded"

I had a look at the CI logs for #6100. The checks from the first few commits show failing tests (i.e. the bug is present) for all macos/linux workflows. And if we look at the NumPy2.0 tests, we can see they also failed here:
https://github.com/pyvista/pyvista/actions/runs/9144044806/job/25141449296?pr=6100#step:12:1137

This confirms that using NumPy 2.0 did not fix this issue, so the condition can be removed. So, the logic could be changed to something like:

      # Integer scalars won't threshold correctly on some systems. Cast to float in these cases.
      has_int_dtype = np.issubdtype(
          array_dtype := self.active_scalars.dtype,
          int,
      ) and array_dtype != np.dtype(np.uint8)
      if has_int_dtype and platform.system() in ['Darwin', 'Linux']:
          self[scalars] = self[scalars].astype(float, casting='safe')

I've now checked a local build with NumPy 1.26.4 and nightly, and I get the exact same warnings (so all the image regression warnings I see are unrelated). Although some warnings could/should be fixed, they don't concern the NumPy version bump, I'll only have to look at the errors (which we've already discussed).

What's the general feeling about fixing unrelated warnings such as these here? Of course only ones that can be fixed trivially. E.g. there's a warning from load_theme() I looked at, and it seemed like a nightmare to fix (short of explicitly silencing it).

What's the general feeling about fixing unrelated warnings such as these here? Of course only ones that can be fixed trivially. E.g. there's a warning from load_theme() I looked at, and it seemed like a nightmare to fix (short of explicitly silencing it).

The warnings from load_theme are indicative that this logic is problematic. I had also noticed this in #5119 (comment). Let's not touch that here. Since you've already touched the code for suppressing warnings for glyph it makes sense if you want to for easier testing here.

I've opened #6210 to temporarily give an upper version pin for our NumPy dependency. Cross-posting my plan here because it's also relevant to this PR:

My plan/proposal is:

1. merge this PR quickly to protect main from the NumPy 2.0 release until we're compatible
2. remove the version restriction in Ensure compatibility with NumPy 2.0 #6156
3. probably make the NumPy nightly build in our CI matrix mandatory, at least until 2.0 is released.

Feedback is welcome on either PR :)

This PR is the critical one to prevent additional regressions we need a green CI for other PRs. I would vote for making the numpy check mandatory in this PR if all the fixes are included. This ensures we don't backslide as we figure out the other pieces.

Thanks @MatthewFlamm, that was also precisely my plan :)

I tested removing the check for numpy version as in
#6156 (comment) and it works for me.

@adeak do you have time to close this out soon? If not, I can make the changes needed to get this green and mandatory in the CI. To make mandatory in the CI, we need to remove the two continue-on-error lines, one is below. This is important, as it is understandable that others are assuming the current green check mark means "Passing".

Thanks @MatthewFlamm, I originally made the test mandatory on my branch, I just reverted that so as not to conflict with your numpy nightly PR.

I also know the version check removal "works", I just wanted to take a closer look at how it's broken without the check (but I probably won't do that).

I can wrap this up on the weekend if that's an acceptable time frame.

This is ready for review.

3.12 with NumPy nightly:

===== 1691 passed, 5 skipped, 2 xfailed, 13 warnings in 1064.07s (0:17:44) =====

Warnings:

=============================== warnings summary ===============================
tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue2]
tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue3]
  /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/wslink/backends/aiohttp/__init__.py:89: NotAppKeyWarning: It is recommended to use web.AppKey instances for keys.
  https://docs.aiohttp.org/en/stable/web_advanced.html#application-s-config
    self.app["state"] = {}

tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue2]
tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue3]
  /home/runner/work/pyvista/pyvista/pyvista/trame/ui/__init__.py:54: UserWarning: Suppress rendering on the plotter is changed to True
    warnings.warn(

tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue2]
tests/plotting/jupyter/test_trame.py::test_trame_plotter_ui[vue3]
  /home/runner/work/pyvista/pyvista/pyvista/trame/ui/__init__.py:54: UserWarning: Suppress rendering on the plotter is changed to False
    warnings.warn(

tests/plotting/test_charts.py::test_chart_common[chart_mpl]
  /home/runner/work/pyvista/pyvista/pyvista/plotting/charts.py:4797: UserWarning: No artists with labels found to put in legend.  Note that artists whose label start with an underscore are ignored when legend() is called with no argument.
    legend = self._fig.axes[0].legend()

tests/plotting/test_picking.py::test_fly_to_right_click
  /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/pytest_pyvista/pytest_pyvista.py:137: UserWarning: pyvista test generated image dir: debug_images does not yet exist.  Creating dir.
    warnings.warn(

tests/plotting/test_plotting.py::test_chart_plot
  /opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/pytest_pyvista/pytest_pyvista.py:218: UserWarning: test_chart_plot Exceeded image regression warning of 200.0 with an image error of 464.9084967320269
    warnings.warn(

tests/plotting/test_theme.py::test_load_theme
tests/plotting/test_theme.py::test_save_before_close_callback
  /home/runner/work/pyvista/pyvista/pyvista/plotting/themes.py:1552: UserWarning: The jupyter_extension_available flag is read only and is automatically detected.
    warnings.warn(

tests/plotting/test_theme.py::test_trame_config
  /home/runner/work/pyvista/pyvista/pyvista/plotting/themes.py:1531: UserWarning: Enabling server_proxy will disable jupyter_extension
    warnings.warn("Enabling server_proxy will disable jupyter_extension")

tests/plotting/test_theme.py::test_trame_config
  /home/runner/work/pyvista/pyvista/pyvista/plotting/themes.py:1567: UserWarning: Enabling jupyter_extension will disable server_proxy
    warnings.warn("Enabling jupyter_extension will disable server_proxy")

I'm not sure why I don't see some of the chart warnings in a local build. But the remaining warnings seem to be

1. trame x8
2. load_theme() x2 that we already discussed is a pain in the ass to fix
3. something about a generated image dir not yet existing (i.e. only affects CI, not an actual runtime warning)
4. chart plot large image regression (bit too large, but I don't see the same issue locally see smaller difference, 304 image error instead of < 200)
5. the aforementioned warning in tests/plotting/test_charts.py::test_chart_common[chart_mpl] that I don't see locally (on Debian linux on Python 3.11, although CI has the same warning for 3.11).

Last message for now: the image regression warning comes from this image

Locally I see the yellow orbit dots in the bottom right subfigure shifting upward a bit. I'm sure this is what leads to the warning, and I guess the same offset is more pronounced in CI. I don't know why this shifts, but it seems harmless enough (and hopefully unrelated to our chart functionality).

Hi @tkoyama010 @banesullivan @MatthewFlamm! Following the merge of this PR, are you expecting to release a new minor version of PyVista that supports Numpy 2.x? Is there any tentative date for that?

Hi @tkoyama010 @banesullivan @MatthewFlamm! Following the merge of this PR, are you expecting to release a new minor version of PyVista that supports Numpy 2.x? Is there any tentative date for that?

Thanks for pointing this. This is labelled as "bug". So I will make patch release for this :)