-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Fix linalg.norm
to handle empty matrices correctly.
#28343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I tested this locally and works fine. Also the types are stable:
has the same output type ( @carlosgmartin Could you add some unit tests for the cases that have been fixed? |
fdacc33
to
853450f
Compare
@eendebakpt Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
853450f
to
660e287
Compare
@eendebakpt Done. |
@carlosgmartin Thanks for the updates. Failing tests seem unrelated, so I approved. The PR needs approval of a numpy dev to be merged though. |
@charris do you have a very clear picture of what the right thing is? @carlosgmartin you seem to have just extended this with from the issue for Also ping @melissawm, maybe you have this type of knowledge more available than me. If there are some norms where an empty matrix default value isn't pretty clear, I would be fine with adding a EDIT: I think this may be an interesting example/reason for why |
So - all of these norms should indeed be 0 on zero matrices. However, some of these norms are the result of computations e.g. "the sum of singular values". In that case it doesn't seem like On the other hand, the initial issue was about empty matrices, not zero-valued matrices. So I'm unsure if this solves that issue. |
@melissawm all of the tests work with empty matrices (so EDIT: Ahhh, zero matrix => zero norm, I suppose also means that empty matrix == empty zero matrix => zero norm. Unless there is some argument to the contrary? |
Oh i see, sorry I looked to quickly. Yes I'd say the default should be 0 for all by following what LAPACK does in *LANGE. |
Mathematically, raising an error for the norm of an empty matrix makes the most sense. What is the use case for doing otherwise? |
@seberg Suppose we are working in a complete lattice The identity element of sup (join) In the case of norms, Furthermore, the matrix where the last equality holds if The norm of a zero vector is, by the definition of a norm, zero. (Note that The fact that the norm of a zero matrix (of which every empty matrix is an example) is zero is also explicitly stated here.
The additional tests are for when
Hard disagree. Mathematically, the answer is well-defined. That'd be like saying that an empty sum or product should raise an error rather than return 0 or 1 respectively.
This is the example that inspired me to create this PR. Other examples might be in the issues I linked to. |
Yes, that is a useful convention for sums and product because of how they are used. But a norm, while it may use sums and products, is not just a sum, it has a meaning. Does the height and width of non-existent object make sense? You can return numbers for both, but what purpose would it serve? If your use case is to answer the question "will it fit in my closet", then returning zero for both makes sense. That is why I am asking about the use case. |
@charris Geometrically, |
@charris I think it's helpful to point out that in many cases NumPy already returns 0 for an empty norm; e.g. In [1]: import numpy as np
In [2]: np.__version__
Out[2]: '2.2.2'
In [3]: np.linalg.norm(np.zeros((0, 0)))
Out[3]: np.float64(0.0)
In [4]: np.linalg.matrix_norm(np.zeros((10, 0, 0)))
Out[4]: array([0., 0., 0., 0., 0., 0., 0., 0., 0., 0.]) So the contribution here is not so much about adding new behavior, but rather about making already existing behavior more consistent. |
I don't think you can argue with the supremum/infinum being 0 or all inf on the set of all possible values for the norm. Unless you can argue that 0 is the correct answer for all (sensible) subsets of matrices, however chosen, having a default return is just wrong. That is exactly the same reason why maximum/minimum raise an error on empty arrays (The user might be working only with values within a specific subset, i.e. range, certainly
|
@seberg The mathematical definition of a norm forces the norm of an empty matrix to be zero. That particular fact has nothing to do with subsets one is working in. |
How can it be that you have |
@seberg According to the docs, |
This point is similar to the discussion in #5032, where the decision was to not return the supremum/infimum in the case of empty |
@jakevdp You mean for the cases that aren't norms, right? |
If it'll expedite merging, I can restrict this PR to only the actual norms, which are uncontroversial and what I'm currently most interested in. |
@carlosgmartin here is the point:
FWIW, I suspect it is in fine, and probably completely correct, to define it to be 0 for all proper norms. But right now the most helpful argument was Melissa pointing to prior art. It may be true that this is mathematical convention or drops out of the definition of the norm. But while 0 being the only possible (and definitely often correct) choice, none of the references I found until now explicitly state anything about empty matrices. |
Carl de Boor. An empty exercise. ACM Signum Newsletter 25 (4), 2-6, 1990.
|
I've edited the PR to restrict changes to the proper norms. |
We discussed this briefly and we decided to give this a try. But it would be good to have release note maybe "change" just in case it surprises someone. (You can find info in |
@seberg Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for popping in after others have already looked this over. I think the release note could use a tweak but otherwise don't have a problem with merging this.
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
@@ -0,0 +1 @@ | |||
* In all cases, ``np.linalg.norm``, ``np.linalg.vector_norm``, and ``np.linalg.matrix_norm`` now return zero for arrays with a shape of zero along at least one axis. Previously, NumPy would raises errors or return zero depending on the shape of the array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* In all cases, ``np.linalg.norm``, ``np.linalg.vector_norm``, and ``np.linalg.matrix_norm`` now return zero for arrays with a shape of zero along at least one axis. Previously, NumPy would raises errors or return zero depending on the shape of the array. | |
* ``np.linalg.norm``, ``np.linalg.vector_norm``, and ``np.linalg.matrix_norm`` now returns zero for empty arrays for most norms chosen by the ``ord`` parameter (empty arrays have at least one axis of size zero). Previously, NumPy raised an error or returned zero depending on the shape of the array. |
@ngoldbaum maybe you can have a quick think (suggest-apply-merge)? Or just merge as is if you prefer...
Actually, maybe the clearest is to just list the changed ones:
* The vector norm ``ord=inf`` and the matrix norms ``ord={1, 2, inf, 'nuc'}``
now always returns zero for empty arrays
[...]
This affects `np.linalg.norm`, `np.linalg.vector_norm`, and `np.linalg.matrix_norm`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I tweaked your suggestion a teeny bit.
Edit: rerunning a failing 32-bit CI test, it failed to start. If it passes I will merge. |
Thanks @carlosgmartin |
Fixes
linalg.norm
to correctly handle empty matrices, which currently raises aValueError
sometimes. For example:This resolves the following issues: