TST: add test cases + implementations for array functions (NEP 18) new in numpy 2.0 by neutrinoceros · Pull Request #15691 · astropy/astropy

neutrinoceros · 2023-12-07T15:51:10Z

Description

Will fix #15690

This isn't finished yet but I'm opening as a draft 10min ahead of the dev call. I'll come back to this later today or tommorow.

Nothing it polished yet but early feedback is welcome nonetheless.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2023-12-07T15:51:39Z

github-actions · 2023-12-07T15:51:45Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

mhvk · 2023-12-07T16:03:21Z

+    SUBCLASS_SAFE_FUNCTIONS |= {np.linalg.cross}
+
+    # doesn't make much sense with quantites ?
+    SUBCLASS_SAFE_FUNCTIONS |= {np.linalg.svdvals}


Maybe note that It works because it just calls svd internally, which transfers the unit (quantities are fine I think)

Note that I actually didn't add a test yet, because the reference implementation lives in SciPy so it's a little bit more involved. But I'm working on it, and I'll add a comment at the same time, thanks for your suggestion !

neutrinoceros · 2023-12-07T17:27:05Z

@mhvk @pllim I'm hesitant to call this a bug fix or a feature, where do you guys think I should add a changelog entry, if any ?

pllim · 2023-12-07T17:28:15Z

This is more like future-proofing maintenance. We usually do not add change log for this. Thanks!

mhvk · 2023-12-07T18:48:33Z

@neutrinoceros - thanks so much for doing this. I'm in a meeting the rest of the week, but probably can find some time for review. Is that useful already, or are you still making changes?

neutrinoceros · 2023-12-07T21:29:26Z

@mhvk I'd like to have allowed_failures go green before I mark this as "open for review", and I'll try to accomplish that tomorrow, but feel free to review at any point ! What I have already is unlikely to change again before your feedback :)

mhvk

OK, had a look now. Quite a few comments, but mostly small.

mhvk · 2023-12-08T03:01:13Z

+
+    @function_helper(module=np.linalg)
+    def outer(x1, x2, /, *args, **kwargs):
+        return (x1, x2) + args, kwargs, x1.unit * x2.unit, None


This won't work, since only one of x1, x2 has to be a Quantity, the other can be a regular array. I'd just copy the relevant parts of np.outer (annoyingly, they have different signature, so we cannot just add to helps).

Note that you should not add the *args, **kwargs here since the numpy equivalent does not take any further arguments.

Aside: a pity the implementation does asarray rather than asanyarray since with the latter this would just have worked... See numpy/numpy#25101 (comment)

Note that you should not add the *args, **kwargs here since the numpy equivalent does not take any further arguments.

The idea is to delegate any unknown arguments to the numpy function that gets called later, since it might gain new arguments in the future. See my other comment. I can still remove those if you're not convinced this has value.

neutrinoceros · 2023-12-08T08:08:09Z

@mhvk A good fraction of your comments are about the need (or lack thereof) for *args + **kwargs. I argued that it's probably best to keep them for forward compatibility, but I'll wait on your reply to decide what to do about it. In the mean time, I'll experiment with the simplifications you hinted to ! Thank you for your early review !

dhomeier · 2023-12-08T18:41:48Z

Haven't gone into the details, but I could verify the fixes are working locally. As of today (8 December) there are 3 more ufuncs added though, np.astype, np.linalg.matmul, np.linalg.tensordot...

neutrinoceros · 2023-12-08T20:06:06Z

Thanks for catching those, surely they'll make the way into nightlies soon. I've added the relevant PRs to the issue's description.
I'll switch this PR to draft while I'm working on these too.

neutrinoceros · 2023-12-08T20:40:36Z

Ok I think I covered everything @dhomeier listed. Opening for review again !

dhomeier · 2023-12-08T21:03:07Z

2.0.0.dev0+git20231206.6ef3089

Obviously updated more frequently than scientific-python-nightly-wheels, but no tensordot yet; perhaps we have to wait till next week.

pllim · 2023-12-08T21:05:59Z

Looks like it should update over the weekend. We can wait.

https://github.com/numpy/numpy/blob/a5b67bbefbf216d325d713f5610bc46d665ff088/.github/workflows/wheels.yml#L24

dhomeier · 2023-12-08T21:09:21Z

But with my git+https://github.com/numpy/numpy.git test_quantity_non_ufuncs.py is now passing!

mhvk · 2023-12-09T01:42:43Z

@neutrinoceros - I think we should add *args, **kwargs only if there are actual arguments to catch. A generic problem is that unless numpy makes things keyword-only, we actually need both, which is not possible if we also have arguments with defaults. Anyway, my sense is generally to try to just mimic the signature.

More concretely, let's just follow precedent in this PR; I think I might quite easily be convinced that it is better to allow more, but best not done in a fixup PR...

neutrinoceros · 2023-12-09T08:26:53Z

More concretely, let's just follow precedent in this PR; I think I might quite easily be convinced that it is better to allow more, but best not done in a fixup PR...

Gotcha. I'll open another issue specifically to discuss this matter. Thank you !

edit: here it is #15703

mhvk

Thanks! One request to reverse to what you had, one potential bug left...

mhvk · 2023-12-09T20:11:24Z

+    def outer(x1, x2, /):
+        # maybe this one can be marked as subclass-safe in the near future ?
+        # see https://github.com/numpy/numpy/pull/25101#discussion_r1419879122
+        return (x1, x2), {}, x1.unit * x2.unit, None


Thanks for adding the comment. Note that this still will break if either x1 or x2 is not a Quantity (which is allowed). You'll sadly need to follow the logic in outer.

Thank you, indeed I forgot to follow up on your first comment. Commit incoming !

neutrinoceros · 2023-12-11T09:35:26Z

rebased, but devdeps CI is still red. I'm not sure whether numpy nightlies are now caught up, so I'm thinking this should wait a little bit longer.

dhomeier · 2023-12-11T16:04:05Z

No, it's at git20231208.a5b67bb now, which should have numpy/numpy#25086 merged.
matmul and tensordot are passing, but there is an additional failure for astype in utils/masked/tests/test_function_helpers.py:

    def test_basic_testing_completeness():
>       assert all_wrapped == (tested_functions | IGNORED_FUNCTIONS | UNSUPPORTED_FUNCTIONS)
E       AssertionError: assert {<function ma...338a130>, ...} == {<function ma...338a130>, ...}
E         Extra items in the left set:
E         <function astype at 0x1033945f0>

neutrinoceros · 2023-12-11T17:49:06Z

Thanks for having a look @dhomeier, I just fixed this one locally so hopefully this is about ready now🤞🏻

mhvk

Looks all good now! Approving but not yet merging, since the PR was still marked as draft.

neutrinoceros · 2023-12-11T20:35:45Z

Yes I just didn't wan't to re-open before I saw CI gone green ! I squashed and rebased, now feel free to merge away !

pllim

Just a small typo. Otherwise LGTM and I am so happy devdeps is green again. Thanks!

…type, unique_all, unique_counts, unique_inverse, unique_values, linalg.cross, linalg.outer, linalg.svdvals, linalg.tensordot, linalg.matmul)

pllim · 2023-12-11T22:05:58Z

Thank you!

…array functions (NEP 18) new in numpy 2.0

…691-on-v6.0.x Backport PR #15691 on branch v6.0.x (TST: add test cases + implementations for array functions (NEP 18) new in numpy 2.0)

neutrinoceros changed the title ~~TST: add test cases for array functions (NEP 18) new in numpy 2.0 (un…~~ TST: add test cases + implementations for array functions (NEP 18) new in numpy 2.0 Dec 7, 2023

github-actions Bot added the units label Dec 7, 2023

mhvk reviewed Dec 7, 2023

View reviewed changes

pllim added this to the v6.0.1 milestone Dec 7, 2023

pllim added numpy-dev backport-v6.0.x no-changelog-entry-needed labels Dec 7, 2023

mhvk reviewed Dec 8, 2023

View reviewed changes

This was referenced Dec 8, 2023

TST: astropy.units completion tests behave differently for functions with unique names #15692

Closed

Add new (2.0) string ufuncs in np.char to UNSUPPORTED_UFUNCS #15699

Merged

neutrinoceros marked this pull request as ready for review December 8, 2023 17:36

neutrinoceros marked this pull request as draft December 8, 2023 20:06

neutrinoceros mentioned this pull request Dec 9, 2023

API [units]: how should we handle forward compatibility against numpy's evolving interfaces ? #15703

Closed

neutrinoceros commented Dec 9, 2023

View reviewed changes

Comment thread astropy/units/quantity_helper/function_helpers.py Outdated

mhvk reviewed Dec 9, 2023

View reviewed changes

neutrinoceros force-pushed the units/new_array_functions branch from 010d160 to 4e57e38 Compare December 11, 2023 08:51

mhvk marked this pull request as ready for review December 11, 2023 19:03

mhvk approved these changes Dec 11, 2023

View reviewed changes

neutrinoceros force-pushed the units/new_array_functions branch from f37349d to bb3bef3 Compare December 11, 2023 20:34

pllim reviewed Dec 11, 2023

View reviewed changes

Comment thread astropy/units/quantity_helper/function_helpers.py Outdated

TST: add test cases for array functions (NEP 18) new in numpy 2.0 (as…

e201579

…type, unique_all, unique_counts, unique_inverse, unique_values, linalg.cross, linalg.outer, linalg.svdvals, linalg.tensordot, linalg.matmul)

neutrinoceros force-pushed the units/new_array_functions branch from 6b6a254 to e201579 Compare December 11, 2023 21:09

pllim approved these changes Dec 11, 2023

View reviewed changes

pllim merged commit 28e593f into astropy:main Dec 11, 2023

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Dec 11, 2023

Backport PR astropy#15691: TST: add test cases + implementations for …

a97e696

…array functions (NEP 18) new in numpy 2.0

meeseeksmachine mentioned this pull request Dec 11, 2023

Backport PR #15691 on branch v6.0.x (TST: add test cases + implementations for array functions (NEP 18) new in numpy 2.0) #15717

Merged

neutrinoceros deleted the units/new_array_functions branch December 11, 2023 22:25

pllim added a commit that referenced this pull request Dec 11, 2023

Merge pull request #15717 from meeseeksmachine/auto-backport-of-pr-15…

c6d7209

…691-on-v6.0.x Backport PR #15691 on branch v6.0.x (TST: add test cases + implementations for array functions (NEP 18) new in numpy 2.0)

Uh oh!

Conversation

neutrinoceros commented Dec 7, 2023

Description

Uh oh!

github-actions Bot commented Dec 7, 2023

Uh oh!

github-actions Bot commented Dec 7, 2023

Uh oh!

mhvk Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Dec 7, 2023

Uh oh!

pllim commented Dec 7, 2023

Uh oh!

mhvk commented Dec 7, 2023

Uh oh!

neutrinoceros commented Dec 7, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

dhomeier commented Dec 8, 2023

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

neutrinoceros commented Dec 8, 2023

Uh oh!

dhomeier commented Dec 8, 2023

Uh oh!

pllim commented Dec 8, 2023

Uh oh!

dhomeier commented Dec 8, 2023

Uh oh!

mhvk commented Dec 9, 2023

Uh oh!

neutrinoceros commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Dec 9, 2023

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

dhomeier commented Dec 11, 2023

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Dec 9, 2023 •

edited

Loading