Skip to content

Add Cholesky function specification #110

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 7 commits into from
May 12, 2021
Merged

Add Cholesky function specification #110

merged 7 commits into from
May 12, 2021

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Jan 14, 2021

This PR

  • specifies the interface for performing Cholesky decomposition.
  • is derived from comparing signatures across array libraries.

Notes

  • NumPy (along with CuPy, JAX, MXNet, TF) does not allow returning either the lower- or upper-triangular Cholesky factor. However, SciPy, Torch, and Dask do support returning either. The ability to return either factor is common outside of the PyData ecosystem (MATLAB, LAPACK). Accordingly, the decision was made to include an upper keyword to support returning the upper-triangular Cholesky factor.

  • Following Torch, MXNet, TF, NumPy, and JAX, this proposal allows for providing a stack of square matrices.

@leofang
Copy link
Contributor

leofang commented Jan 27, 2021

It is straightforward for CuPy to support the upper keyword, as internally we delegate to cuSOLVER/rocSOLVER which supports both modes: https://docs.nvidia.com/cuda/cusolver/index.html#cuSolverDN-lt-t-gt-potrf

@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 3 times, most recently from 0607525 to 138e963 Compare April 19, 2021 20:25
@kgryte
Copy link
Contributor Author

kgryte commented May 12, 2021

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

@kgryte kgryte merged commit 76730f1 into main May 12, 2021
@kgryte kgryte deleted the cholesky branch May 12, 2021 02:56
@lucascolley
Copy link
Member

What was the motivation for choosing the upper=False default here?

@kgryte
Copy link
Contributor Author

kgryte commented Aug 21, 2024

@lucascolley See the API comparison linked to in the OP: https://github.com/data-apis/array-api-comparison/blob/0459e5dd51fa38df8bf24363f4fa5895ac5c2929/signatures/linalg/cholesky.md

TL;DR: the kwarg was not universally supported. Among array libraries, PyTorch had upper=False, so we followed PyTorch.

@lucascolley
Copy link
Member

Thanks @kgryte . cc @mdhaber for awareness

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.

4 participants