Skip to content
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

BUG: common: fix cholesky upper decomp for complex dtypes #61

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

lucascolley
Copy link
Contributor

Closes gh-54.

cc @asmeurer

@lucascolley lucascolley changed the title BUG: fix cholesky upper decomp for complex dtypes BUG: common: fix cholesky upper decomp for complex dtypes Sep 22, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @lucascolley! I verified the change on your NumPy PR, and this matches that one. In it goes.

@rgommers rgommers enabled auto-merge (squash) January 3, 2024 19:22
@rgommers rgommers disabled auto-merge January 3, 2024 19:23
@rgommers
Copy link
Member

rgommers commented Jan 3, 2024

Actually, let me re-run CI since a PyTorch job was red and I couldn't see the CI log anymore.

@rgommers rgommers enabled auto-merge (squash) January 3, 2024 19:25
@lucascolley
Copy link
Contributor Author

thanks Ralf. I wonder whether any changes are needed to array-api-tests since this wasn't caught initially?

@rgommers
Copy link
Member

rgommers commented Jan 3, 2024

I wonder whether any changes are needed to array-api-tests since this wasn't caught initially?

I checked quickly, and yes indeed - upper is tested, but only with real-valued dtypes (via floating_dtypes from hypothesis). I guess the fix is to use complex_dtypes from https://hypothesis.readthedocs.io/en/latest/_modules/hypothesis/extra/array_api.html. Are you interested to follow up on that?

@rgommers
Copy link
Member

rgommers commented Jan 3, 2024

Several failures due to fft testing mainly, I assume because new tests were added in the test suite and not yet here.

All the linalg.cholesky tests passed, so I'll go ahead and merge this.

@rgommers rgommers disabled auto-merge January 3, 2024 20:33
@rgommers rgommers merged commit 88814e5 into data-apis:main Jan 3, 2024
7 of 18 checks passed
@lucascolley lucascolley deleted the cholesky-fix branch January 3, 2024 20:34
@lucascolley
Copy link
Contributor Author

Are you interested to follow up on that?

I had a look but I'm not sure how to fix. Will open an issue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: common: linalg.cholesky returns the conjugate of the expected upper decomposition
2 participants