Skip to content

Comments

BUG: Fix norm type promotion#17709

Merged
rossbar merged 6 commits intonumpy:mainfrom
toslunar:fix-norm-type-promotion
Apr 7, 2022
Merged

BUG: Fix norm type promotion#17709
rossbar merged 6 commits intonumpy:mainfrom
toslunar:fix-norm-type-promotion

Conversation

@toslunar
Copy link
Contributor

@toslunar toslunar commented Nov 4, 2020

Reopen #10667:

I tested np.linalg.norm with the following code, but it seems the type is not preserved correctly.

import numpy as np

for dtype in [np.float16, np.float32, np.float64]:
    a = np.ones((2,), dtype=dtype)
    ret = np.linalg.norm(a, 3, None, False)
    if not (dtype == ret.dtype):
        print('expected', dtype, 'but got', ret.dtype)

Without this fix (NumPy 1.14.1):

expected <class 'numpy.float16'> but got float64
expected <class 'numpy.float32'> but got float64

With NumPy 1.14.1 + this fix, nothing should be printed.

Related to #10368 and cupy/cupy#875 (comment)

@toslunar
Copy link
Contributor Author

toslunar commented Nov 4, 2020

Without the bugfix, the new tests fail:

% python runtests.py -t numpy/linalg/tests/test_linalg.py
Building, see build.log...
Build OK
NumPy version 1.20.0.dev0+bef7aa0
NumPy relaxed strides checking option: True
NumPy CPU features: SSE SSE2 SSE3 SSSE3* SSE41* POPCNT* SSE42* AVX* F16C* FMA3* AVX2* AVX512F? AVX512CD? AVX512_KNL? AVX512_SKX? AVX512_CLX? AVX512_CNL? AVX512_ICL?

(snip)

FAILED numpy/linalg/tests/test_linalg.py::TestNormDouble::test_vector_return_type - AssertionError:
FAILED numpy/linalg/tests/test_linalg.py::TestNormSingle::test_vector_return_type - AssertionError:
FAILED numpy/linalg/tests/test_linalg.py::TestNormInt64::test_vector_return_type - AssertionError:
3 failed, 290 passed, 1 skipped, 28 deselected, 1 xfailed, 1 warning in 5.12s

@mattip
Copy link
Member

mattip commented Nov 4, 2020

Windows is failing.

@toslunar
Copy link
Contributor Author

toslunar commented Nov 4, 2020

Windows is failing at ord=2. Thus, it seems some function in the fast path (maybe sqnorm = dot(x, x) or ret = sqrt(sqnorm)) unintentionally changes the dtype from longdouble to float64.

@toslunar
Copy link
Contributor Author

toslunar commented Nov 4, 2020

type(np.sqrt(np.longdouble(2))) returns np.float64 on Windows. I'll relax the test to compare dtypes, instead of comparing .dtype.types.

@toslunar toslunar force-pushed the fix-norm-type-promotion branch from bef7aa0 to 4f0c803 Compare November 4, 2020 10:17
BvB93
BvB93 reviewed Nov 4, 2020
View reviewed changes
@toslunar
Copy link
Contributor Author

toslunar commented Nov 5, 2020

It seems the fixed tests test_vector_return_type, test_matrix_return_type should have been methods of TestNorm_NonSystematic because they don't respect self.dt. May I refactor it?

BvB93
BvB93 reviewed Nov 5, 2020
View reviewed changes
@BvB93
Copy link
Member

BvB93 commented Nov 5, 2020

It seems the fixed tests test_vector_return_type, test_matrix_return_type should have been methods of TestNorm_NonSystematic because they don't respect self.dt. May I refactor it?

By the looks of it TestNorm_NonSystematic appears to hold various miscellaneous tests.
Based on this I feel that the current test arrangement is marginally more suitable, despite them ignoring self.dt.

@toslunar toslunar force-pushed the fix-norm-type-promotion branch from 32c8ad8 to 4f0c803 Compare November 6, 2020 03:02
@toslunar
Copy link
Contributor Author

toslunar commented Nov 6, 2020

It seems the fixed tests test_vector_return_type, test_matrix_return_type should have been methods of TestNorm_NonSystematic because they don't respect self.dt. May I refactor it?

By the looks of it TestNorm_NonSystematic appears to hold various miscellaneous tests.
Based on this I feel that the current test arrangement is marginally more suitable, despite them ignoring self.dt.

That's true. TestNorm_NonSystematic is not the place. But there was a minor issue: repeating the tests three times in subclasses. How about a288d826b5a603cbffa7bd1a1849a378c049b8a0?

@toslunar toslunar force-pushed the fix-norm-type-promotion branch from 1a4d0ed to c51d2c8 Compare November 6, 2020 09:36
eric-wieser
eric-wieser reviewed Nov 6, 2020
View reviewed changes
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

I think this probably needs a compatibility release note, but otherwise looks good to me

@toslunar
Copy link
Contributor Author

toslunar commented Nov 9, 2020

This part of the release note of 1.14.0 is for the issue this PR fixes. ord in [Inf, -Inf, 0, 1] (and [2, None] with axis) were fixed then, but 2.001 was not.

@toslunar toslunar force-pushed the fix-norm-type-promotion branch from 72c9bbf to 2e23e50 Compare November 10, 2020 01:37
Base automatically changed from master to main March 4, 2021 02:05
@InessaPawson InessaPawson added the triage review Issue/PR to be discussed at the next triage meeting label Mar 20, 2022
@seberg
Copy link
Member

seberg commented Apr 6, 2022

We discussed this today at the meeting and I think the general consensus is that this can probably go in. It may be good to have a quick look over the changes.
In general, it could make sense to do the promotion (i.e. return float64), but that should align with all other code paths (and probably things like sum and prod if that happens).

@rossbar rossbar self-assigned this Apr 6, 2022
@InessaPawson InessaPawson added triaged Issue/PR that was discussed in a triage meeting and removed triage review Issue/PR to be discussed at the next triage meeting labels Apr 6, 2022
rossbar
rossbar approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I took the liberty of updating the release note, but everything else LGTM and CI is green (minus circleCI, for unrelated reasons). Thanks @toslunar !

@rossbar rossbar merged commit 0d13f9f into numpy:main Apr 7, 2022
@toslunar
Copy link
Contributor Author

toslunar commented Apr 8, 2022

Thank you for the reviews and improving the release note!

@toslunar toslunar deleted the fix-norm-type-promotion branch April 8, 2022 08:14
melissawm pushed a commit to melissawm/numpy that referenced this pull request Apr 12, 2022
@toslunar @kmaehashi
@rossbar @melissawm
BUG: Consistent promotion for norm for all values of ord (numpy#17709)
8fb8b2a 
Previously, numpy.linalg.norm would return values with the same floating-point
type as input arrays for most values of the ``ord`` parameter, but not all.
This PR fixes this so that the output dtype matches the input for all (valid) values
of ``ord``.

Co-authored-by: Kenichi Maehashi <webmaster@kenichimaehashi.com>
Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
@asmeurer asmeurer mentioned this pull request Jun 15, 2022
Merged
@joanrue joanrue mentioned this pull request Nov 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

@mattip mattip mattip left review comments

@BvB93 BvB93 BvB93 left review comments

@rossbar rossbar rossbar approved these changes

Assignees

@rossbar rossbar

Labels

00 - Bug component: numpy.linalg triaged Issue/PR that was discussed in a triage meeting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@toslunar @mattip @BvB93 @seberg @eric-wieser @rossbar @charris @InessaPawson @kmaehashi