MAINT: Fix computation of numpy.array_api.linalg.vector_norm by asmeurer · Pull Request #21084 · numpy/numpy

asmeurer · 2022-02-18T01:39:10Z

There were several issues due to the fact that it was mostly untested in the array API test suite.

This also fixes #21083. I'm unclear exactly how I should add a test for this.

For the array API vector_norm, tests are being added to the array API test suite.

Various pieces were incorrect due to a lack of complete coverage of this function in the array API test suite.

Previously it would always give float64 because an internal calculation involved a NumPy scalar and a Python float. The fix is to use a 0-D array instead of a NumPy scalar so that it type promotes with the float correctly. Fixes numpy#21083 I don't have a test for this yet because I'm unclear how exactly to test it.

seberg

A few small comments/suggestions but looks fine. Maybe @honno can sign this off?

seberg · 2022-02-18T21:53:54Z

numpy/array_api/linalg.py

+        if a.ndim == 0:
+            a = a[None]
+        else:
+            a = a.flatten()


I don't understand, flatten should always return a 1-D array?

EDIT: Actually, why not .ravel()? flatten forces a copy and that is really not necessary (ravel copies more often than you would expect too, but at least not always.)

Oh I never realized that about flatten. We have to make it a 1-D array to force it to do a vector norm (this weird shape-based behavior is why we split norm into vector_norm and matrix_norm in the array API).

Yes, but np.array(0).ravel() (or flatten) also is 1D, this is not <= 1 but == 1, so ravel() covers the a[None] path just as well.

seberg · 2022-02-18T21:56:16Z

numpy/array_api/linalg.py

+            (np.prod([a.shape[i] for i in axis], dtype=int), *[a.shape[i] for i in rest]))
+        _axis = 0
+    else:
+        _axis = axis


We do have a normalize_axis_tuple helper in numpy/core/numeric.py, maybe worthwhile?

Ah, I should have guessed there are probably already helpers in NumPy to do some of these things. I'll see if I can find anything that makes this simpler.

seberg · 2022-02-18T22:02:20Z

numpy/array_api/linalg.py

+        elif isinstance(axis, int):
+            _axis = (axis,)
+        else:
+            _axis = axis


Might put this together with the setup (i.e. figuring out the result shape there? But looks fine.

asmeurer · 2022-02-18T22:24:25Z

@seberg any thoughts on how I can add a test for #21083?

seberg · 2022-02-18T22:29:57Z

Frankly, no, this is almost certainly ancient code from the early days of NumPy (or possibly older!), I would just git grep for norm tests and add one next to it where it seems reasonable.

asmeurer · 2022-02-18T22:37:13Z

Yeah, I figured out where the norm tests are. I just am really unclear how exactly they work. Just reading them it looks to me like this ought to already be tested (specifically here).

Actually, looking closer now, I think I see the problem, which is that the test here doesn't actually test the output dtype, just that it is a floating dtype.

seberg · 2022-02-18T22:43:20Z

On first glance, I think the tests are just buggy. _TestNormGeneral doesn't seem to actually use self.dt in the relevent tests? And even then, I am not sure its checks would be sufficient (but didn't dig in).

It would be nice to just replace this stuff with pytest.parametrize, that would be much clearer to read than this inheritance dance.

asmeurer · 2022-02-18T23:05:11Z

I agree. I don't know if this sort of thing is standard in the NumPy test suite or if it's just a weird leftover thing from ages ago that hasn't been modernized.

seberg · 2022-02-18T23:10:54Z

Just a weird leftover, probably it was a good and typical pattern at some point. I would not mind if it was just parametrized even if that is a biggish diff, on first sight, it looks straight forward enough.

asmeurer · 2022-02-19T00:25:56Z

I also didn't realize those test are also testing integer inputs to norm() (which aren't even in scope in the array API). So testing the correct dtype is a little more complicated. Is there a helper function to get the correct promoted type?

Also, I hope this isn't a problem, but I'm probably not going to refactor those tests to use parameterization. It would make a good first issue for someone to do that, but I really just want to add a test for my fix if it's not too complicated.

seberg · 2022-02-19T00:33:17Z

Is there a helper function to get the correct promoted type?

I dunno np.result_type(dtype1, dtyp2, 0.)? I am not sure what the current logic ends up doing exactly, though. I mean, promotion does depend on the operation in principle, i.e. the maximum norm should preserve dtype in principle, but other norms should promote to floating.

Also, I hope this isn't a problem, but I'm probably not going to refactor those tests to use parameterization. It would make a good first issue for someone to do that, but I really just want to add a test for my fix if it's not too complicated.

nah sure, but doesn't hurt to hope ;).

honno · 2022-02-21T09:59:11Z

@seberg's comments on using ravel() and the existing axes utils (if possible) make sense, otherwise LGTM. Personally I prefer figuring out result shapes after the fact, as you can follow up the reshape straight after to reduce cognitive load.

asmeurer · 2022-02-25T23:40:27Z

I think fiddling with the existing linalg tests is going to be a deeper rabbit hole than I really want to get down for this PR. If you want I can add a simple, separate regression test for the norm() fix. Otherwise, I would suggest opening a "good first contribution" issue for refactoring those tests to use pytest parameterization, and also to more explicitly test the result type in each case.

I have cleaned up the implementation of numpy.array_api.vector_norm a little bit as per the review comments.

seberg · 2022-06-14T23:43:05Z

Unfortunately there is now a (probably very small) merge conflict. @honno otherwise you were happy with this and we should just merge it?

honno · 2022-06-15T08:38:09Z

Unfortunately there is now a (probably very small) merge conflict. @honno otherwise you were happy with this and we should just merge it?

Yep LGTM other then merge conflicts. Again @seberg's previous suggestions on ravel() and exploring existing axis are worth exploring, but it'd be nice to just get this in.

However let's just see if @asmeurer is okay with this PR still, as there have been more questions and clarifications on the linalg extension in the spec since this PR was opened.

asmeurer · 2022-06-15T20:34:26Z

Looks like #21083 was fixed by #17709 so my fix is no longer needed. No opinion on which of the fixes is better. I guess neither would be needed once type promotion is cleaned up. At least that PR was (apparently) able to add tests.

asmeurer · 2022-06-15T20:36:53Z

The actual fix in the array_api is still relevant. I have fixed the conflicts.

seberg · 2022-06-16T00:43:59Z

Lets put it in then. Thanks!

Patch from numpy/numpy#21084

asmeurer added 2 commits February 17, 2022 18:02

Fix computation of numpy.array_api.linalg.vector_norm

71fe19a

Various pieces were incorrect due to a lack of complete coverage of this function in the array API test suite.

asmeurer mentioned this pull request Feb 18, 2022

More improvements to test_linalg data-apis/array-api-tests#101

Merged

seberg reviewed Feb 18, 2022

View reviewed changes

Clean up the numpy.array_api.linalg.vector_norm code a little bit

284a7c0

charris changed the title ~~Fix computation of numpy.array_api.linalg.vector_norm~~ MAINT: Fix computation of numpy.array_api.linalg.vector_norm Feb 20, 2022

github-actions bot added the 03 - Maintenance label Feb 20, 2022

charris closed this Apr 6, 2022

charris reopened this Apr 6, 2022

Merge branch 'main' into array-api-vector_norm-fix

54e1184

seberg merged commit 70026c4 into numpy:main Jun 16, 2022

leofang pushed a commit to leofang/cupy that referenced this pull request Jul 29, 2022

MAINT: Fix computation of numpy.array_api.linalg.vector_norm

a407b90

Patch from numpy/numpy#21084

leofang mentioned this pull request Jul 29, 2022

Update cupy.array_api (cont'd) cupy/cupy#6932

Merged

Uh oh!

Comments

Conversation

asmeurer commented Feb 18, 2022

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

seberg Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmeurer Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

seberg Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

seberg Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

asmeurer Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

seberg Feb 18, 2022

Choose a reason for hiding this comment

Uh oh!

asmeurer commented Feb 18, 2022

Uh oh!

seberg commented Feb 18, 2022

Uh oh!

asmeurer commented Feb 18, 2022

Uh oh!

seberg commented Feb 18, 2022

Uh oh!

asmeurer commented Feb 18, 2022

Uh oh!

seberg commented Feb 18, 2022

Uh oh!

asmeurer commented Feb 19, 2022

Uh oh!

seberg commented Feb 19, 2022

Uh oh!

honno commented Feb 21, 2022

Uh oh!

asmeurer commented Feb 25, 2022

Uh oh!

seberg commented Jun 14, 2022

Uh oh!

honno commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmeurer commented Jun 15, 2022

Uh oh!

asmeurer commented Jun 15, 2022

Uh oh!

seberg commented Jun 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seberg Feb 18, 2022 •

edited

Loading

honno commented Jun 15, 2022 •

edited

Loading