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

fix torch frontend blas_and_lapack_ops cholesky #28767

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

Daniel4078
Copy link
Contributor

PR Description

Related Issue

Closes #28766

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials

@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 19, 2024

locally, for the 5 frontend tests of test_torch_cholesky, only one of paddle fails with RuntimeError('(NotFound) The kernel with key (CPU, Undefined(AnyLayout), bfloat16) of kernel cholesky is not registe...cond.end() && kernel_key.backend() == Backend::CPU:1 == true:1.] (at /paddle/paddle/phi/core/kernel_factory.cc:262)\n')

@Daniel4078 Daniel4078 marked this pull request as ready for review June 19, 2024 04:46
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 19, 2024

Now it seems like the target test_torch_cholesky is no longer failing according to the github pipeline

Copy link
Contributor

@Sam-Armstrong Sam-Armstrong left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Daniel4078!

I've just had a look at the failing test for torch.linalg.cholesky, and it seems the problem is actually with the test itself. I was able to fix it for all backends by changing this line:

x = np.matmul(x.T, x) + np.identity(x.shape[0])

to this:

x = np.matmul(np.conjugate(x.T), x) + np.identity(x.shape[0], dtype=dtype[0])

Would you be able to make this change, and see if it also works for the version in blas_and_lapack_ops? (you can remove all these current changes you made) Thanks!

ivy/functional/frontends/torch/blas_and_lapack_ops.py Outdated Show resolved Hide resolved
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 19, 2024

@Sam-Armstrong Thank you! I have checked, your changes also work in this blas_and_lapack_ops version. And I think this PR should be ready to be merged. Just in case, have you already pushed the fix to the linalg version or should I include it in this PR?

Copy link
Contributor

@Sam-Armstrong Sam-Armstrong left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot @Daniel4078!

@Sam-Armstrong Sam-Armstrong merged commit 5e71687 into ivy-llc:main Jun 19, 2024
3 of 5 checks passed
@Daniel4078 Daniel4078 deleted the fix-torch-cholesky branch June 20, 2024 00:48
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.

Fix Frontend Failing Test: torch - blas_and_lapack_ops.cholesky and torch - linalg.cholesky
2 participants