BUG: Have `norm` cast non-floating point arrays to 64-bit float arrays #7088

jakirkham · 2016-01-21T18:28:23Z

Simply ensures that non-floating type arrays passed to norm get cast to some form of float.

madphysicist · 2016-01-21T18:37:27Z

numpy/linalg/linalg.py

@@ -2112,6 +2112,9 @@ def norm(x, ord=None, axis=None, keepdims=False):
    """
    x = asarray(x)

+    if not issubclass(x.dtype.dtype, floating):


This looks like a copy-and-double-paste typo. The tests are failing here. Did you mean x.dtype?

Yep, sorry, was on autopilot. Fixed. Meant x.dtype.type.

madphysicist · 2016-01-21T18:48:12Z

numpy/linalg/linalg.py

@@ -2112,6 +2112,9 @@ def norm(x, ord=None, axis=None, keepdims=False):
    """
    x = asarray(x)

+    if not issubclass(x.dtype.type, floating):


One more thing. Did you mean either float or numpy.float? This is the new point of failure in the tests.

Nope meant floating, but it wasn't imported. Just fixed as you were typing. :) See above.

I don't think this is the right check. Typically, we handle this by adding 0.0 to the result, which preserves inexact types and respects complex. Note

In [10]: a = array([1,2,3], complex) In [11]: issubclass(a.dtype.type, floating) Out[11]: False

You can also do

In [13]: issubdtype(a.dtype, np.inexact) Out[13]: True

Ah, you want to check complexfloating too. Sure, sorry wasn't thinking about complex.

madphysicist · 2016-01-21T18:49:48Z

numpy/linalg/linalg.py

@@ -2112,6 +2112,9 @@ def norm(x, ord=None, axis=None, keepdims=False):
    """
    x = asarray(x)

+    if not issubclass(x.dtype.type, floating):
+        x = x.astype(float)


On a related note, should this be np.float or am I misreading?

np.float is just an alias to float (i.e. np.float is float will be True). In fact, I believe those aliases are planned to be removed, but I can't find the issue right now.

jakirkham · 2016-01-21T19:16:40Z

Should we try to fix np.dot too?

jakirkham · 2016-01-21T19:23:01Z

So, ord=0 will still return int currently. Do we want to make it float there too? This would be simple, but am unsure as to whether this is a good idea.

charris · 2016-01-21T19:36:05Z

Hmm, interesting question. Consistency would say "yes", so I tend that way, but let's see if @pv has an opinion, it is his addition.

charris · 2016-01-21T20:25:37Z

numpy/linalg/linalg.py

@@ -2112,6 +2112,9 @@ def norm(x, ord=None, axis=None, keepdims=False):
    """
    x = asarray(x)


I'd just add 0.0 here.

That is x = asarray(x) + 0.0. it's a bit ugly, but there you go. I had a proposal to add a mintype keyword to asarray that would also have solved this problem.

Personally, I'm not a fan. I think that adding or multiplying is performing an otherwise needless element operation especially for arrays of the right type. Also, if we need to cast to a preferred type, we should just do that explicitly.

jakirkham · 2016-01-21T23:11:24Z

Just to clarify the conflict I see with ord=0.

From the documentation side, it says it returns ndarray or float so this change would follow with that.
However, the examples return ints for ord=0. So, we would be in disagreement there.

From a mathematical standpoint, we know ord=0 will always be a positive semi-definite whole number. So, it seems unnecessary to make it a float. However, our experience with integer rollover tells us otherwise.

charris · 2016-01-22T05:07:18Z

LGTM, could you put some test together to check the types? Can make some simple for loops to cycle through the types and norms and check the result type.

jakirkham · 2016-01-22T18:32:44Z

Ok, I think these work. There are a few cases where things might get cast up or converted from complex to float so it got a bit messy, but these check out for me thus far.

jakirkham · 2016-01-22T18:41:32Z

Should we try folding the exact and inexact tests together? I probably can do this by reusing the inexact case for the exact case.

jakirkham · 2016-01-22T18:54:28Z

Also, just as an FYI, we have a serious storm front forecasted to hit our area in about 2hrs so there is a chance I might lose power/internet when or soon after it hits. If you need me to clean up anything before then, please let me know as soon as you can. Otherwise, I may have to let this sit for a few days depending on conditions.

charris · 2016-01-22T19:52:33Z

If you disappear for a few snow days and the PR needs some fixes, I'll pull it down and do the fix. To avoid snow, buy a snow blower, it seems to affect the weather. Winters here have been milder ever since I bought mine...

charris · 2016-01-22T22:56:23Z

numpy/linalg/tests/test_linalg.py

+        for each_inexact_types in all_types:
+            at = a.astype(each_inexact_types)
+
+            n = norm(at, -np.inf)


Just to note that it is unusual to use any of ijklmn for floats. That goes back to the implicit typing of early Fortran where variables beginning with those letters were integers.

I suppose. Though I do see some others doing the same thing in this file ( https://github.com/numpy/numpy/blob/master/numpy/linalg/tests/test_linalg.py#L935 ).

I have renamed all the ns to ans.

I expect the knee jerk reaction to 'ijklmn' will eventually pass with my generation. Fortran bugs due to incorrectly typed variables were subtle and frustrating. Explicit typing in Fortran was a lifesaver when it arrived, right up there with the arrival of prototypes in C.

I guess I am more of a stickler for i, j, and sometimes k being ints . The rest of them sort of fall of the map for me. Since learning Python, I find myself using itertools.product for any complicated nested for loop. Sometimes use l for lists that I can't think of a better name for.

…l computations are done with floating point numbers.

jakirkham · 2016-01-22T23:55:39Z

BTW, as I have rebased and forced pushed a bunch while cleaning this up, it appears the AppVeyor queue has really blocked up with pointless jobs from old version of this PR. I think all but the most recent one can be canceled.

charris · 2016-01-22T23:56:26Z

LGTM. I'm going to clear out a bunch of the AppVeyor tests that are queued up, otherwise we will wait forever for test completion.

jakirkham · 2016-01-23T00:00:46Z

Sounds good. Looks like the 1.11.0 list is basically done. When are you thinking of tagging the release?

charris · 2016-01-23T00:10:22Z

Sometime this weekend, maybe tomorrow.

jakirkham · 2016-01-23T00:28:56Z

Cool. One other side question. Do the doctests get run as part of the testing suite and, if not, should they?

charris · 2016-01-23T00:41:26Z

No, they don't. It comes up for discussion now and then, the last time being last summer. I think you can find it the mailing list archives. We don't use doctests for testing because they are hard to maintain and difficult to use for many tests, they mostly serve as pedagogical examples. So while it doesn't hurt to keep doctests up to date -- we would not refuse a PR fixing a doctest -- it isn't a high priority.

BUG: Have `norm` cast non-floating point arrays to 64-bit float arrays

charris · 2016-01-23T01:28:46Z

Thanks @jakirkham .

jakirkham · 2016-01-23T01:30:14Z

Thanks @charris. I'll backport it.

jakirkham · 2016-01-23T01:34:09Z

Backported to 1.11.x in this PR ( #7098 ).

charris · 2016-01-23T01:43:32Z

For future reference, your commit messages lack line breaks. Commit messages should have hard line breaks and line length <=72, blank line after first link commit summary.

jakirkham · 2016-01-23T01:46:34Z

Thanks, I'll keep that in mind for the future.

mgeier · 2016-04-27T13:01:11Z

This introduced a regression, see #7575.

As of numpygh-7088, x is always a float array anyway

jakirkham mentioned this pull request Jan 21, 2016

numpy.linalg.norm() broken for multidimensional integer arrays #5626

Closed

madphysicist reviewed Jan 21, 2016
View reviewed changes

jakirkham force-pushed the cast_float_linalg branch from 988aeda to 18cb568 Compare January 21, 2016 18:39

madphysicist reviewed Jan 21, 2016
View reviewed changes

jakirkham force-pushed the cast_float_linalg branch from 18cb568 to 393d7a8 Compare January 21, 2016 18:48

madphysicist reviewed Jan 21, 2016
View reviewed changes

jakirkham force-pushed the cast_float_linalg branch from 393d7a8 to 5aeb667 Compare January 21, 2016 19:00

charris added 00 - Bug component: numpy.linalg labels Jan 21, 2016

charris added this to the 1.11.0 release milestone Jan 21, 2016

jakirkham force-pushed the cast_float_linalg branch from 5aeb667 to ca1102e Compare January 21, 2016 19:05

charris reviewed Jan 21, 2016
View reviewed changes

jakirkham force-pushed the cast_float_linalg branch 7 times, most recently from 49dfdc5 to 1a6ac8b Compare January 22, 2016 18:31

jakirkham force-pushed the cast_float_linalg branch from 1a6ac8b to 5b9c65e Compare January 22, 2016 20:27

charris reviewed Jan 22, 2016
View reviewed changes

jakirkham force-pushed the cast_float_linalg branch from 62c7db6 to 284e1ed Compare January 22, 2016 22:58

jakirkham added 3 commits January 22, 2016 18:15

TST: Verify that norm is properly casting values to floats as needed.

bc4a17e

BUG: Make sure that the ord=0 case returns a float.

43c6a89

DOC: Update norm docstring to include examples that reflect that al…

75d5b59

…l computations are done with floating point numbers.

jakirkham force-pushed the cast_float_linalg branch from 284e1ed to 75d5b59 Compare January 22, 2016 23:16

charris added a commit that referenced this pull request Jan 23, 2016

Merge pull request #7088 from jakirkham/cast_float_linalg

66156a0

BUG: Have `norm` cast non-floating point arrays to 64-bit float arrays

charris merged commit 66156a0 into numpy:master Jan 23, 2016

jakirkham deleted the cast_float_linalg branch January 23, 2016 01:30

jakirkham mentioned this pull request Jan 23, 2016

Backport 7088, BUG: Have norm cast non-floating point arrays to 64-bit float arrays #7098

Merged

charris removed this from the 1.11.0 release milestone Jan 23, 2016

mgeier mentioned this pull request Apr 29, 2016

Regression in linalg.norm() using dtype=object #7575

Closed

WarrenWeckesser mentioned this pull request Sep 18, 2016

numpy.linalg.norm returns nan for an array of int16 #6128

Closed

eric-wieser mentioned this pull request Aug 6, 2017

BUG: Fix problems with obj2sctype #9522

Merged

eric-wieser added a commit to eric-wieser/numpy that referenced this pull request Aug 6, 2017

MAINT: Remove unneeded asfarray

351fe97

As of numpygh-7088, x is always a float array anyway

WarrenWeckesser mentioned this pull request Oct 24, 2021

numpy.linalg.norm doesn't work well with int16 inputs #4851

Closed

		@@ -2112,6 +2112,9 @@ def norm(x, ord=None, axis=None, keepdims=False):
		"""
		x = asarray(x)

BUG: Have norm cast non-floating point arrays to 64-bit float arrays #7088

BUG: Have norm cast non-floating point arrays to 64-bit float arrays #7088

Conversation

jakirkham commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Jan 21, 2016

jakirkham commented Jan 21, 2016

charris commented Jan 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Jan 21, 2016

charris commented Jan 22, 2016

jakirkham commented Jan 22, 2016

jakirkham commented Jan 22, 2016

jakirkham commented Jan 22, 2016

charris commented Jan 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Jan 22, 2016

charris commented Jan 22, 2016

jakirkham commented Jan 23, 2016

charris commented Jan 23, 2016

jakirkham commented Jan 23, 2016

charris commented Jan 23, 2016

charris commented Jan 23, 2016

jakirkham commented Jan 23, 2016

jakirkham commented Jan 23, 2016

charris commented Jan 23, 2016

jakirkham commented Jan 23, 2016

mgeier commented Apr 27, 2016

BUG: Have `norm` cast non-floating point arrays to 64-bit float arrays #7088

BUG: Have `norm` cast non-floating point arrays to 64-bit float arrays #7088