MAINT: (additional) array copy semantics shims #20172
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Fixes scipygh-20170 * restore the old `copy` behavior when using NumPy >= `2.0.0dev0` for now; I think Mateusz was using `asarray` to solve the same problem more concisely previously--I'm not bothered if folks prefer that, this was just more obviously restoring the original behavior to me (at least, I think it is...) * `test_raise_non_numeric_gh18254` was mocking an array-like object so it gained an otherwise unused `copy` argument for compliance; same goes for `FakeArray2` in FFT testing (and in other places that use arr mocking after that...) * old shims using `np._CopyMode` are not safe to use (well, they appear to be more complex than just bools now) * some non-mock objects (I think) like `LowRankMatrix` and `Jacobian` were also given a `copy` argument for arr to play along, though I haven't thought about the deeper consequences of doing that * `array_api_strict` needs a `copy` shim or two as well I think; that's external and not in scope here I don't think, but will cause this to fail until then: `test_dispatch_to_unrecognize_library` * apart from that one test vs. NumPy `main`, the rest of the suite passed with NumPy `main` and `1.26.4` on this branch in my hands [skip circle] [skip cirrus]
tylerjereddy
requested review from
andyfaff,
ev-br,
ilayn,
larsoner,
perimosocordiae and
peterbell10
as code owners
github-actions
bot
added
scipy.fftpack
scipy.stats
scipy.io
scipy.linalg
scipy.sparse
scipy.optimize
scipy.interpolate
scipy.spatial
scipy.fft
Cython
tylerjereddy
changed the title
Contributor
@tylerjereddy Let's keep it all here in one PR! I see that |
lucascolley
removed request for
andyfaff,
ev-br,
ilayn,
larsoner,
perimosocordiae and
peterbell10
lucascolley
removed
scipy.io
scipy.linalg
scipy.sparse
scipy.optimize
scipy.interpolate
scipy.spatial
scipy.fft
Cython
Also make the code to deal with changes in NumPy around the `copy` keyword as concise as possible; do the version checks only in a single place. This also resolved linting errors in the previous commit. [skip cirrus]
This is required, not doing so is deprecated. Unclear why it doesn't always emit a warning. Also reduce overparametrization of a stats test that used `__array__`. [skip cirrus]
Member
|
Thanks @tylerjereddy. I added some changes to:
|
Member
|
Remaining CI failure is a real complaint from Mypy |
…dent [skip cirrus] [skip circle]
Member
Fixed now. This is probably best squash-merged once CI is happy. I'm away for most of the day, but if whoever does the merge could also hit the "Run workflow" button on https://github.com/scipy/scipy/actions/workflows/wheels.yml after merging, that would be very useful. |
lucascolley
reviewed
Mar 2, 2024
scipy/optimize/_nonlin.py
Outdated
| self.collapse() | ||
|
|
||
| def __array__(self): | ||
| def __array__(self, copy=None): |
Member
There was a problem hiding this comment.
dtype=None is used in every other signature change in the PR but not here - is that deliberate? Otherwise LGTM
Member
There was a problem hiding this comment.
Good catch. Tests didn't fail and my code search skills apparently fell short.
This code doesn't really seem to be used (beyond internally in one other class, so dtype/copy cannot be passed), I'll add the keyword with a warning that any input that's not None is not handled.
Member
|
We have several broken CI jobs and wheels to this, so I hit the green button. Restarting wheel builds now. Thanks @tylerjereddy and @lucascolley! |
This was referenced Mar 3, 2024
erykoff
added a commit
to erykoff/fitsio
that referenced
this pull request
May 17, 2024
Following the instructions in https://numpy.org/devdocs/numpy_2_0_migration_guide.html#adapting-to-changes-in-the-copy-keyword I used scipy/scipy#20172 as a template for setting copy=False or copy=None for numpy 1.x and numpy 2.x.
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.
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes Test failures due to
asarray(..., copy=False)semantics change in numpy main #20170restore the old
copybehavior when using NumPy >=2.0.0dev0for now; I think Mateuszwas using
asarrayto solve the same problem more concisely previously--I'm not bothered if folks prefer that, this was just more obviously restoring the original behavior to me (at least, I think it is...)test_raise_non_numeric_gh18254was mocking an array-like object so it gained an otherwiseunused
copyargument for compliance; same goesfor
FakeArray2in FFT testing (and in otherplaces that use arr mocking after that...)
old shims using
np._CopyModeare not safe to use (well, they appear to be more complexthan just bools now)
some non-mock objects (I think) like
LowRankMatrixandJacobianwere also given acopyargument for arr to play along, though I haven't thought about the deeper consequences of doing thatarray_api_strictneeds acopyshim or two as well I think; that's external and not in scope here I don't think, but will cause this to fail until then:test_dispatch_to_unrecognize_libraryapart from that one test vs. NumPy
main, the rest of the suite passed with NumPymainand1.26.4on this branch in my hands[skip circle] [skip cirrus]
@mtsokol I'm happy to back out the stuff you already covered in gh-20171, I think that's just a tiny subset of this though, so shouldn't be too disruptive.