Skip to content

Fix array data copy behavior for numpy 1 and 2 consistency#6932

Merged
andy-sweet merged 18 commits intonapari:mainfrom
andy-sweet:fix-numpy2-array-copy
May 30, 2024
Merged

Fix array data copy behavior for numpy 1 and 2 consistency#6932
andy-sweet merged 18 commits intonapari:mainfrom
andy-sweet:fix-numpy2-array-copy

Conversation

@andy-sweet
Copy link
Copy Markdown
Member

@andy-sweet andy-sweet commented May 24, 2024

References and relevant issues

Failure in our prerelease tests which picked up numpy==2.0.0rc2: #6871 (comment)

Related discussion on Zulip: https://napari.zulipchat.com/#narrow/stream/212875-general/topic/handling.20the.20numpy.202.2E0.20release

Description

This fixes the coercion of data using the Array pydantic custom class so that it works for both numpy 1 and 2, and only makes a copy if needed.

Instead we could consider using np.asarray, but we'll also need to replace the ndmin argument (which np.asarray does not have), by using np.expand_dims or similar. I have done that it in one other place where I don't need those fancy arguments.

# behavior associated with ndmin.
# https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword
copy = None
if np.lib.NumpyVersion(np.__version__) < '2.0.0b1':
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's probably a better comparison than this, but I'm sleepy and this is the best I've got right now.

@andy-sweet
Copy link
Copy Markdown
Member Author

There are other uses of np.array(..., copy=False, ...) in our code. I think we could replace most of those with np.asarray (or as here if needed) as I doubt we ever want to error if a copy must be made.

@codecov
Copy link
Copy Markdown

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.42%. Comparing base (07dbc0a) to head (a4a16ba).

Files Patch % Lines
napari/utils/events/custom_types.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6932      +/-   ##
==========================================
- Coverage   92.47%   92.42%   -0.06%     
==========================================
  Files         611      611              
  Lines       55157    55160       +3     
==========================================
- Hits        51007    50981      -26     
- Misses       4150     4179      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jni
Copy link
Copy Markdown
Member

jni commented May 24, 2024

There are other uses of np.array(..., copy=False, ...) in our code. I think we could replace most of those with np.asarray

💯

@jni jni added this to the 0.5.0 milestone May 24, 2024
@jni jni added the bugfix PR with bugfix label May 24, 2024
@jni
Copy link
Copy Markdown
Member

jni commented May 27, 2024

@andy-sweet are you planning on changing those in this PR?

@andy-sweet
Copy link
Copy Markdown
Member Author

@andy-sweet are you planning on changing those in this PR?

I'm out for the next 24 hours, but should be able do that soon after. May as well get it done in this PR, so yes!

@andy-sweet
Copy link
Copy Markdown
Member Author

My process here was to grep/search for copy=False and find usage with np.array and similar.

The following are some similar usages that I did not change.

There is also some usage of np.array(..., copy=False, ...) in colors.py vendored from matplotlib. I did not change these because they have not been changed upstream in matplotlib. If these cause an issue, we could just vendor their changes or re-vendor the whole module.

pre-commit-ci bot and others added 11 commits May 28, 2024 20:30
# Description
As menu actions are all one file (see napari#6848 and napari#6883), update the file
docstrings to remove mention of 'non-qt' actions.
# Description
There are now no more 'non-qt' providers and processors since napari#6743
Amended some wording to clarify.
[napari-error-reporter](https://github.com/tlambert03/napari-error-reporter)
is archived and unmaintained, and the project is removed from sentry.
This PR removes the few lines that looked for the plugin if it was
installed

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…apari#6912)

# References and relevant issues

Closes napari#6597 

# Description

Enable proper handling of mouse bindings (single and double click) with
unfinished/invalid shapes definitions (not enough vertices to close the
shape or trying to add the same vertex multiple times. With the changes:

* Ignore addition of a vertex (mouse single click) if it has the same
position as the last vertex added (fix for napari#6597)
* Trying to finish the drawing (mouse double click) of an invalid shape
causes the in progress shape to be removed (fix for
napari#6912 (comment))

Preview:


![invalid_shapes](https://github.com/napari/napari/assets/16781833/8002862b-4cb6-4b13-80e2-366b0c4f4d03)
…6256 (napari#6836)

# References and relevant issues
Supposed to Close napari#6256. I am not sure about the stacklevel arg of the
warning.


# Description
Added DeprecationWarning if arguments passed and removed the
functionality related to them within the functions.

# Regarding checklist
I am not sure about the docs repo.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Wouter-Michiel Vierdag <michiel.vierdag@embl.de>
Co-authored-by: Wouter-Michiel Vierdag <w-mv@hotmail.com>
# References and relevant issues

Closes napari#6934

# Description

Incorrect callback was being used for the `Open Folder` action. This PR
fixes it. We need a testing framework for the actions and menus
@lucyleeow, which we knew, but maybe we should prioritize ahead of
converting the layer actions.
…ng the plugin manager (napari#6931)

Fixes napari/napari-plugin-manager#26

# Description

Catches connectivity errors when fetching for plugin information when
opening the plugin manager.

Although the issue lives in the plugin manager repo, the code that
fetches the code still lives in napari, hence with the PR seem to be
better resolved on this side.

# Result

## Before this PR


![napari-before](https://github.com/napari/napari/assets/3627835/6e704463-2dd3-4701-9aa5-93f2501e48fd)


## With this PR


![napari](https://github.com/napari/napari/assets/3627835/ffa3ad0a-da18-4e76-a4aa-8a007f90ce36)

The warning reads: 

>There seems to be an issue with network connectivity. Remote plugins
cannot be installed, only local ones.
napari#6929)

# Description

Iterating on a performance PR that needs benchmarks is currently more
cumbersome than it needs to be: add the run-benchmarks label, get a
performance report, act on the report (ie make code changes and push),
then, to run the benchmarks again, a core dev needs to *manually remove*
the label, then immediately add it again. Instead, this PR automates the
removal of the label once the benchmarks are done running and the report
has been generated. In fact, it should also remove the label even if the
benchmarks fail due to some bug in the code or benchmark code.
)

# References and relevant issues
 Towards napari#6516

# Description
Remove `_submenus.py` file, in favour of defining them with their
actions.

I am not 100% happy with this though because some menu actions will have
a qt-actions and non-qt-actions file, in which case it would be again
confusing for the dev to guess where the submenus are defined.

Luckily the 3 menus that do have submenus (layer, file and view) only
have one action file.

Note I have had to move submenu registration inside `init_qactions`.

Not sure what to do here, open to suggestions and happy to close this as
well.

---------

Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
@github-actions github-actions bot added tests Something related to our tests qt Relates to qt maintenance PR with maintance changes, labels May 28, 2024
@andy-sweet
Copy link
Copy Markdown
Member Author

Pending CI checks, this should be ready now. I pulled out the numpy version check into the module import, so that it's only executed once. This definition could also be moved to somewhere like napari.utils.misc if needed elsewhere.

@andy-sweet andy-sweet changed the title Fix Array data copy behavior for numpy 1 and 2 consistency Fix array data copy behavior for numpy 1 and 2 consistency May 28, 2024
@andy-sweet
Copy link
Copy Markdown
Member Author

Note that I don't expect this to fix the pre release tests. There are at least some vispy issues lurking as mentioned on Zulip: https://napari.zulipchat.com/#narrow/stream/212875-general/topic/handling.20the.20numpy.202.2E0.20release/near/440434390

@andy-sweet andy-sweet added the ready to merge Last chance for comments! Will be merged in ~24h label May 28, 2024
@andy-sweet andy-sweet merged commit 40ac1fb into napari:main May 30, 2024
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label May 30, 2024
@andy-sweet andy-sweet deleted the fix-numpy2-array-copy branch May 30, 2024 16:40
jni pushed a commit that referenced this pull request Jun 13, 2024
# References and relevant issues
I don't see a tracking issue for numpy 2.0 support, but here are some
previous PRs I've found:
#6932
#6776

And a zulip thread:

https://napari.zulipchat.com/#narrow/stream/212875-general/topic/handling.20the.20numpy.202.2E0.20release/near/381330412

# Description
This fixes a few more numpy-2.0 related issues. I will comment on some
of them inline.

I tested locally using [a wheel from my vispy
PR](https://github.com/vispy/vispy/actions/runs/9424977460?pr=2599). I
also uninstalled `tensorstore` to skip related tests (see
google/tensorstore#165).

There are still a few test failures in
`napari/layers/image/_tests/test_image.py` that look possibly related to
Xarray:
```
napari/layers/image/_tests/test_image.py:649 test_image_scale[scale5] - DeprecationWarning: __array__ implementation doesn't accept a copy keyword, so passing copy=False failed. __array__ must implement 'dtype' and… [1012/3861]
```

I also get a failure on
`napari/_qt/widgets/_tests/test_qt_tooltip.py::test_qt_tooltip_label`,
but I think this may be a macOS thing (it passes if I mouse over it).

Other than that, local tests with `pytest napari` are all green.
brisvag pushed a commit to vispy/vispy that referenced this pull request Jun 17, 2024
This introduces numpy 2.0 support. napari has started looking into
support, and ran into a few small issues using vispy testing against it.
See the [napari zulip
thread](https://napari.zulipchat.com/#narrow/stream/212875-general/topic/handling.20the.20numpy.202.2E0.20release/near/381330412)
for some discussion and relevant links.

The process for adding support in this PR was:
* Run the ruff numpy 2.0 support check and fix:
```
ruff check vispy --select NPY201
ruff check vispy --select NPY201 --fix
```
* Run tests, fix issues and warnings
* Update for changes to behavior of the `copy` kwarg in `np.array`
* Add `vispy.util.np_copy_if_needed` value (`False` for numpy < 1.28,
`None` for numpy >= 2.0)
    * Search for `copy=False` used in `no.array` and remove it
* Search for `copy=copy` used in `np.array`, replace with
`copy=vispy.util.np_copy_if_needed`
    * See references for more information
* [NumPy 2.0 Migration
Guide](https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword)
* [SciPy PR doing the same](scipy/scipy#20172)
* [napari PR doing the same](napari/napari#6932)
* Update build configuration for Cython code
* Previously, it was important to link against the oldest compatible
numpy version to ensure
      forward compatibility. This was accomplished by depending on

[oldest-supported-numpy](https://pypi.org/project/oldest-supported-numpy/).
* Now, you can just depend on `numpy>=2.0.0rc1`, and the build will be
compatible with both 1.x
      and 2.x.
* This is updated in the `[build-system]` section of `pyproject.toml`
* See [Depending on
NumPy](https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice)
for more information


I think this is ready for review, but I'm going to try creating a wheel
and using to run napari tests with numpy 2.0 as well. I'll mark it
non-draft after I do that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR with bugfix maintenance PR with maintance changes, qt Relates to qt tests Something related to our tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants