API: Adjust ``linalg.pinv`` and ``linalg.cholesky`` to Array API by mtsokol · Pull Request #25388 · numpy/numpy

mtsokol · 2023-12-13T17:13:03Z

This PR is (one of) the last Array API compatibility changes:

rtol parameter added to np.linalg.pinv as an alternative to rcond for bc (both can't be provided at the same time) (rtol will still diverge from Array API wrt. default value but at least it can be passed as a keyword argument)
upper parameter added to np.linalg.cholesky
Adjusted array-api-skips.txt to remove redundant skips

ngoldbaum · 2023-12-19T15:33:36Z

numpy/linalg/_linalg.py

+    (symmetric if real-valued) and positive-definite. No checking is performed
+    to verify whether `a` is Hermitian or not. In addition, only the lower or
+    upper-triangular and diagonal elements of `a` are used. Only `L` or `U`
+    is actually returned.


I think all of these should be double backtick quotes, since they're not cross-references to numpy types.

Right, I formatted them.

ngoldbaum · 2023-12-19T15:33:59Z

doc/release/upcoming_changes/25388.new_feature.rst

+--------------------------------------------------------------
+
+``upper`` keyword parameter was added to `numpy.linalg.cholesky` and
+``rtol`` keyword parameter to `numpy.linalg.pinv` for Array API compatibility.


Weren't both added for array API compat?

I'd also mention that rtol doesn't have the array API default.

That's correct, both of these were added to array-api-compat.

rtol doesn't have the default value because default for pinv is always taken from rcond (for backward compatibility) - I added a note about it.

I'm trying to say it reads like only rtol was added for compat now. Maybe something like:

The ``upper`` and ``rtol`` keyword parameters were added to `numpy.linalg.colesky` and `numpy.lingalg.ping`, respectively, to improve array API compatibility.

Ah, now I got it - I updated it.

ngoldbaum · 2023-12-19T15:37:18Z

numpy/linalg/_linalg.py

    a, wrap = _makearray(a)
+    if rtol is not None:
+        if rcond != 1e-15:
+            raise ValueError("`rtol` and `rcond` can't be both set.")


What if someone sets rcond=1e-15 and rtol != None. In that case rtol would be ignored. Of course that's a pretty unusual thing to happen.

Still, it might be better to change the default value of rcond to None, and if both are unset, assume the user meant rcond=1e-15 to preserve backward compatibility. Also document that behavior.

Or error if both are set to non-default values and leave the default alone.

Sure! Changed to rcond=None.

numpy/linalg/_linalg.py

mhvk

One little buglet and a comment.

Bit more generally, might it make sense to move towards the array API and deprecate rcond? (maybe just a FutureDeprecationWarning or so).

Also, might it be an idea to use rtol=np._NoValue as the default for now, to allow passing in rtol=None to opt in to the array API default?

Not sure about either, to be honest.

mhvk · 2023-12-19T19:01:17Z

numpy/linalg/_linalg.py

                  over='ignore', divide='ignore', under='ignore'):
        r = gufunc(a, signature=signature)
+    if upper:
+        r = r.T.conj()


This is only correct for 2-d arrays, while I think the input can be a stack of matrices. Replace with

r = matrix_transpose(r).conj()

Ah, right - fixed!

mhvk · 2023-12-19T19:05:04Z

numpy/linalg/_linalg.py

+    rtol : (...) array_like of float, optional
+        Same as `rcond`, but it's an Array API compatible parameter name.
+        Only `rcond` or `rtol` can be set at a time. If none of them are
+        provided then ``1e-15`` default is used.


Needs a .. versionadded:: 2.0.0

Should note here that if None is passed in, the API standard default is used.

mtsokol · 2023-12-20T12:20:11Z

Bit more generally, might it make sense to move towards the array API and deprecate rcond? (maybe just a FutureDeprecationWarning or so).

Also, might it be an idea to use rtol=np._NoValue as the default for now, to allow passing in rtol=None to opt in to the array API default?

Not sure about either, to be honest.

@mhvk In my opinion it's a good idea - I added a PendingDeprecationWarning when rcond is set explicitly and made it possible to use rtol default value with rtol=None.

mhvk · 2023-12-20T15:24:33Z

numpy/linalg/_linalg.py

                  over='ignore', divide='ignore', under='ignore'):
        r = gufunc(a, signature=signature)
+    if upper:
+        r = matrix_transpose(r).conj()


Now wondering about this, since this makes a copy of a potentially large matrix: an alternative would be to actually define cholesky_up (like done for eigenvalues, etc.), i.e., creating a new gufunc by adding to

numpy/numpy/linalg/umath_linalg.cpp

Lines 1988 to 1993 in 0032ede

static void

cholesky_lo(char **args, npy_intp const *dimensions, npy_intp const *steps,

void *NPY_UNUSED(func))

{

cholesky<typ>('L', args, dimensions, steps);

}

numpy/numpy/linalg/umath_linalg.cpp

Line 4178 in 0032ede

GUFUNC_FUNC_ARRAY_REAL_COMPLEX__(cholesky_lo);

numpy/numpy/linalg/umath_linalg.cpp

Lines 4390 to 4399 in 0032ede

{

"cholesky_lo",

"(m,m)->(m,m)",

"cholesky decomposition of hermitian positive-definite matrices. \n"\

"Broadcast to all outer dimensions. \n"\

" \"(m,m)->(m,m)\" \n",

4, 1, 1,

FUNC_ARRAY_NAME(cholesky_lo),

equal_2_types

},

If it's Ok, let me create later a separate issue/PR that introduces a new ufunc.

Sure, it is fine to have the support now and improve it later!

mhvk

Beyond the larger separate comment about maybe creating the appropriate gufunc to avoid copies, a few nits in-line.

mhvk · 2023-12-20T15:25:39Z

numpy/linalg/_linalg.py

+    rtol : (...) array_like of float, optional
+        Same as `rcond`, but it's an Array API compatible parameter name.
+        Only `rcond` or `rtol` can be set at a time. If none of them are
+        provided then ``1e-15`` default is used.


Should note here that if None is passed in, the API standard default is used.

mhvk · 2023-12-20T15:26:00Z

doc/release/upcoming_changes/25388.new_feature.rst

+improve array API compatibility.
+
+For `numpy.linalg.pinv` if neither ``rcond`` nor ``rtol`` is specified,
+the ``rcond``'s default is used.


Maybe worth stating that eventually we hope to deprecate rcond?

Sure! Added.

mhvk · 2023-12-20T15:28:56Z

numpy/linalg/tests/test_linalg.py

+def test_pinv_rtol_arg():
+    a = np.array([[1, 2, 3], [4, 1, 1], [2, 3, 1]])
+
+    with suppress_warnings() as sup:


Since the warning is intensional, I'd do

with pytest.warns(PendingDeprecationWarning, match="In the future `rcond`"):

As the warning got removed it's not needed.

numpy/linalg/_linalg.py

mhvk

All looks good to me!

ngoldbaum · 2023-12-21T16:27:08Z

Thanks @mtsokol!

mtsokol self-assigned this Dec 13, 2023

github-actions bot added the 30 - API label Dec 13, 2023

mtsokol added the Numpy 2.0 API Changes label Dec 13, 2023

mtsokol force-pushed the pinv-cholesky-array-api branch from a0e0fdf to c56cc1b Compare December 13, 2023 17:34

mtsokol mentioned this pull request Dec 13, 2023

Tracking issue: Add array API standard support to the main namespace #25076

Closed

23 tasks

mtsokol force-pushed the pinv-cholesky-array-api branch 2 times, most recently from c0c5104 to cfcf694 Compare December 15, 2023 13:27

ngoldbaum reviewed Dec 19, 2023

View reviewed changes

numpy/linalg/_linalg.py Show resolved Hide resolved

mtsokol force-pushed the pinv-cholesky-array-api branch from cfcf694 to a220d6a Compare December 19, 2023 17:07

mhvk reviewed Dec 19, 2023

View reviewed changes

mtsokol force-pushed the pinv-cholesky-array-api branch 3 times, most recently from 294e51b to 3f5cf73 Compare December 20, 2023 11:59

mhvk reviewed Dec 20, 2023

View reviewed changes

charris changed the title ~~API: Adjust linalg.pinv and linalg.cholesky to Array API~~ API: Adjust linalg.pinv and linalg.cholesky to Array API Dec 20, 2023

ngoldbaum reviewed Dec 20, 2023

View reviewed changes

numpy/linalg/_linalg.py Outdated Show resolved Hide resolved

mtsokol force-pushed the pinv-cholesky-array-api branch from 3f5cf73 to 32e8eac Compare December 20, 2023 17:51

mhvk approved these changes Dec 20, 2023

View reviewed changes

API: Adjust linalg.pinv and linalg.cholesky to Array API

dfe7fd6

mtsokol force-pushed the pinv-cholesky-array-api branch 3 times, most recently from 700a78f to dfe7fd6 Compare December 21, 2023 11:42

mtsokol added this to the 2.0.0 release milestone Dec 21, 2023

ngoldbaum merged commit 5038cef into numpy:main Dec 21, 2023

mtsokol deleted the pinv-cholesky-array-api branch December 21, 2023 16:33

mtsokol mentioned this pull request Dec 22, 2023

ENH: Add a dedicated cholesky_up ufunc #25457

Closed

mhvk mentioned this pull request Dec 22, 2023

MAINT: use cholesky_up gufunc for upper Cholesky decomposition #25473

Merged

	static void
	cholesky_lo(char *args, npy_intp const dimensions, npy_intp const *steps,
	void *NPY_UNUSED(func))
	{
	cholesky<typ>('L', args, dimensions, steps);
	}

	{
	"cholesky_lo",
	"(m,m)->(m,m)",
	"cholesky decomposition of hermitian positive-definite matrices. \n"\
	"Broadcast to all outer dimensions. \n"\
	" \"(m,m)->(m,m)\" \n",
	4, 1, 1,
	FUNC_ARRAY_NAME(cholesky_lo),
	equal_2_types
	},

Uh oh!

Comments

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum Dec 19, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtsokol commented Dec 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtsokol commented Dec 13, 2023 •

edited

Loading

ngoldbaum Dec 19, 2023 •

edited

Loading