Skip to content

Add specification for computing the pseudo-inverse (linalg: pinv) #118

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

Merged
merged 15 commits into from
May 12, 2021
Merged

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Jan 25, 2021

This PR

  • specifies the interface for computing the (Moore-Penrose) pseudo-inverse.
  • is derived from comparing signatures across array libraries.

Notes

  • Following Torch, MXNet, TF, NumPy, and JAX, this proposal allows for providing a stack of square matrices. CuPy does not currently support providing stacks.

  • Dask does not provide an API for computing the pseudo-inverse.

  • TF supports a validate_args argument for embedding additional validations within its computational graph.

  • NumPy, MXNet, and Torch (latest master) supporting providing a hermitian keyword argument to indicate that more efficient computation methods be used. This PR omits this keyword, as more of an implementation detail, than a generalizable API.

  • NumPy, MXNet, CuPy, and Torch set the default rcond value to 1e-15, while JAX and TF compute a default value based on the machine epsilon associated with the input array data type and the number of rows/cols. This PR follows JAX and TF in computing the default value (as 1e-15 does not make sense for non-float64 input, such as float32 or bfloat16) and requiring that rcond be a broadcast compatible array (or a float).

  • This proposal renames the rcond keyword argument to rtol in order to unify keyword arguments for pinv, lstsq, and matrix_rank which all support specifying relative tolerances. The default value is also the same across these APIs.

  • Question: should this return a namedtuple to allow for a variable number of returns (see API for variable number of returns in linalg #95)? SciPy, e.g., does support returning multiple values (the matrix B along with the effective rank of the result). In theory, other info could be returned, such as error info, but not clear whether this is enough of a forward-looking concern.

    • Answer: no.

@rgommers
Copy link
Member

Question: should this return a tuple to allow for a variable number of returns (see #95)? SciPy, e.g., does support returning multiple values (the matrix B along with the effective rank of the result). In theory, other info could be returned, such as error info, but not clear whether this is enough of a forward-looking concern.

I'd say no. We should avoid variable number of returns as much as possible. Returning a tuple doesn't help at all; changing the tuple length would still be a serious backwards compat break.

I'll comment on gh-95, after looking at some of these PRs that's worth reconsidering.

@rgommers
Copy link
Member

rgommers commented Jan 26, 2021

NumPy, MXNet, CuPy, and Torch set the default rcond value to 1e-15, while JAX and TF compute a default value based on the machine epsilon associated with the input array data type and the number of rows/cols. This PR follows JAX and TF in computing the default value (as 1e-15 does not make sense for non-float64 input, such as float32 or bfloat16) and requiring that rcond be a broadcast compatible array.

The default value here is given as 10.0 * max(M, N) * eps. I see a couple of potential issues:

@kgryte
Copy link
Contributor Author

kgryte commented Jan 28, 2021

Re: 10.0 factor. Yeah, I am not sure the reasoning behind JAX and TF's use of the factor. That factor is absent from lstsq because the factor is not used by NumPy and was not clear to me whether the rcond defaults for pinv and lstsq should match.

@kgryte
Copy link
Contributor Author

kgryte commented Jan 28, 2021

Re: namedtuple. Another alternative is to simply return a dictionary.

@kgryte
Copy link
Contributor Author

kgryte commented Feb 16, 2021

Renamed rcond to tol in order to unify similar keyword arguments across pinv, lstsq, and matrix_rank. Removed 10.0 scaling factor; the default tolerances are now computed consistently across each of the three aforementioned APIs.

Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Just a quick comment 🙂

@kgryte
Copy link
Contributor Author

kgryte commented Mar 4, 2021

Renamed tol to rtol to more explicitly indicate relative tolerance and pave the way for future specification evolution (e.g., atol).

@leofang
Copy link
Contributor

leofang commented Mar 5, 2021

btw CuPy will support batched pinv in the upcoming v9.0 (cupy/cupy#4686).

@rgommers rgommers added the API extension Adds new functions or objects to the API. label Mar 20, 2021
@rgommers rgommers force-pushed the main branch 2 times, most recently from 2f8f5e4 to 0607525 Compare April 19, 2021 20:22
@kgryte
Copy link
Contributor Author

kgryte commented May 12, 2021

Thanks, @leofang, for the review! This is ready for merge...

@kgryte kgryte merged commit 9893373 into main May 12, 2021
@kgryte kgryte deleted the pinv branch May 12, 2021 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants