Skip to content

Add Cholesky function specification#110

Merged
kgryte merged 7 commits intomainfrom
cholesky
May 12, 2021
Merged

Add Cholesky function specification#110
kgryte merged 7 commits intomainfrom
cholesky

Conversation

@kgryte
Copy link
Copy Markdown
Contributor

@kgryte kgryte commented Jan 14, 2021 •
edited
Loading

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
Copy Markdown
Contributor

leofang commented Jan 27, 2021 •
edited
Loading

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

leofang
leofang reviewed Mar 19, 2021
View reviewed changes
@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
Copy Markdown
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
@kgryte kgryte mentioned this pull request Jun 6, 2022
Merged
@lucascolley
Copy link
Copy Markdown
Member

lucascolley commented Aug 21, 2024

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

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

lucascolley commented Aug 21, 2024

Thanks @kgryte . cc @mdhaber for awareness

@mdhaber mdhaber mentioned this pull request Sep 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leofang leofang leofang approved these changes

Assignees

No one assigned

Labels

API extension Adds new functions or objects to the API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kgryte @leofang @lucascolley @rgommers