Skip to content

Add Cholesky function specification - #110

Merged
kgryte merged 7 commits into
mainfrom
cholesky
May 12, 2021
Merged

Add Cholesky function specification#110
kgryte merged 7 commits into
mainfrom
cholesky

Conversation

@kgryte

@kgrytekgryte commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

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

leofang commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

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

Comment threadspec/API_specification/linear_algebra_functions.md Outdated
@rgommersrgommers added the API extension Adds new functions or objects to the API. label Mar 20, 2021
@rgommers
rgommersforce-pushed the main branch 3 times, most recently from 0607525 to 138e963CompareApril 19, 2021 20:25
@kgryte

Copy link
Copy Markdown
ContributorAuthor

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

@kgryte
kgryte merged commit 76730f1 into mainMay 12, 2021
@kgryte
kgryte deleted the cholesky branch May 12, 2021 02:56
@lucascolley

Copy link
Copy Markdown
Member

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

@kgryte

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Member

Thanks @kgryte . cc @mdhaber for awareness

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API extensionAdds new functions or objects to the API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kgryte@leofang@lucascolley@rgommers