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 svd #28770

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Daniel4078
Copy link
Contributor

PR Description

Related Issue

Closes #28769

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

Jin Wang added 2 commits June 20, 2024 10:47
… functions,

update the docstring of ivy.svd to mention the change of content of return when compute_uv is False,
update svd's torch backend to only compute the necessary second component to be more efficient and the relevant docstring in ivy.svd as well
…ple definition and behavior when compute_uv is false
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 20, 2024

@Sam-Armstrong Can you tell me about how to get the device of the input received by the torch frontend function svd? Because the documentation requires the zero-filled matrixes in the output should be on the same device as input (https://pytorch.org/docs/stable/generated/torch.svd.html).
Also, why does the ground truth of the svd test shown here (https://github.com/Transpile-AI/ivy/actions/runs/9589504921/job/26443402936#step:6:184) is not in the correct form of (U,S,V) but only contain two matrixs?

@Sam-Armstrong
Copy link
Contributor

@Daniel4078 you should just be able to do x.device right? I'm not sure about the ground truth, but it should be produced by the native torch function, so you should be able to reproduce the results manually in a python script

…ple definition and behavior when compute_uv is false
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 20, 2024

@Sam-Armstrong I found something strange when I run the test locally. The test's error is like this

values = [[array([[0., 0.],
       [0., 0.]], dtype=float32), array([[4.89516249e-308, 0.00000000e+000],
       [4.89516249e-30...    [4.89516249e-308, 0.00000000e+000]]), array([[[0., 0.],
        [0., 0.]],

       [[0., 0.],
        [0., 0.]]])]]
this_key_chain = None

    def assert_same_type_and_shape(values, this_key_chain=None):
        x_, y_ = values
        for x, y in zip(x_, y_):
            if isinstance(x, np.ndarray):
                x_d = str(x.dtype).replace("longlong", "int64")
                y_d = str(y.dtype).replace("longlong", "int64")
>               assert (
                    x.shape == y.shape
                ), f"returned shape = {x.shape}, ground-truth returned shape = {y.shape}"
E               AssertionError: returned shape = (2, 2), ground-truth returned shape = (2, 2, 2)
E               Falsifying example: test_torch_svd(
E                   on_device='cpu',
E                   frontend='torch',
E                   backend_fw='jax',
E                   dtype_and_x=(['float64'], [array([[[-2.44758124e-308, -2.44758124e-308],
E                             [-2.44758124e-308, -2.44758124e-308]],
E                     
E                            [[-2.44758124e-308, -2.44758124e-308],
E                             [-2.44758124e-308, -2.44758124e-308]]])]),
E                   some=False,
E                   compute=False,
E                   test_flags=FrontendFunctionTestFlags(
E                       num_positional_args=0,
E                       with_out=False,
E                       with_copy=False,
E                       inplace=False,
E                       as_variable=[False],
E                       native_arrays=[False],
E                       test_trace=False,
E                       test_trace_each=False,
E                       generate_frontend_arrays=False,
E                       transpile=False,
E                       precision_mode=True,
E                   ),
E                   fn_tree='ivy.functional.frontends.torch.svd',

Here the return from both the ground truth function and ourfrontend are named tuples (U,S,V) where U and V should be zero matrixs (just like what is in the "values" variable). It seems like the assertion check somehow pick the first two part of one returned tuple and consider them as the frontend-groundtruth pair. I think maybe we should investigate this further as x_, y_ = values does not seem to work correctly here.

@Daniel4078 Daniel4078 marked this pull request as ready for review June 22, 2024 03:11
@Sam-Armstrong
Copy link
Contributor

@Daniel4078 isn't the problem here that the shape of the tensors in the named tuple is different to the ground truth? Like the first element U of the tuple is 3d in the ground truth but 2d in the function return

@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 30, 2024

@Daniel4078 isn't the problem here that the shape of the tensors in the named tuple is different to the ground truth? Like the first element U of the tuple is 3d in the ground truth but 2d in the function return

@Sam-Armstrong I see, comparing to the document of ivy.svd, it seems like the document of torch.svd assume the input is 2D and is incomlete.

…fixed the wrong shape and dtype of return. Now there are somehow numerial difference between return of groundtruth torch.svd and ivy.svd
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jun 30, 2024

@Sam-Armstrong the tests now sometime fails due to the fact that the correct output of svd is not unique and I am thinking of copying the test for ivy.linalg.svd(

# value test based on recreating the original matrix and testing the consistency
) instead as it test values by doing actual calcuations. Do you agree of this approach? And should I also apply this fix to other failing frontend svd functions in this PR?

@Sam-Armstrong
Copy link
Contributor

@Daniel4078 sure, think that makes sense. yeah that would be great if you can fix the other failing frontend svd tests in this PR, thanks!

@Daniel4078 Daniel4078 force-pushed the fix-torch-frontend-blas_and_lapack_ops.svd branch from ef20966 to dce10a6 Compare July 2, 2024 04:18
@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jul 2, 2024

@Daniel4078 sure, think that makes sense. yeah that would be great if you can fix the other failing frontend svd tests in this PR, thanks!

It is quite surprising that now torch.blas_and_lapack_ops.svd, torch.tensor.svd, jax.lax.linalg.svd, jax.numpy.linalg.svd, numpy.linalg.decompositions.svd are failing; while torch.linalg.svd , tensorflow.linalg.svd and tensorflow.raw_ops.svd are skipped. The only passing svd test is that of ivy functional API core.

@Sam-Armstrong
Copy link
Contributor

@Daniel4078 you haven't made any changes to the tests here, did you force-push over them?

@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jul 3, 2024

@Daniel4078 you haven't made any changes to the tests here, did you force-push over them?

I accidently commited some other changes intended in another branch in here, so I thought it would be better to remove that commit.

@Daniel4078
Copy link
Contributor Author

Daniel4078 commented Jul 3, 2024

@Sam-Armstrong Why all the torch.svd frontend tests are skipping when I run it locally, showing that "Skipped: Function not implemented in backend." What should I do in this case?

@Daniel4078
Copy link
Contributor Author

trying to solve the TypeError: iteration over a 0-d array error when comparing two (somehow empty) result from the matric calculation

… instead of the normal tuple and apply the np.asarray(x) change over ivy.as_numpy in other tests, ensure the different return pattern (s,u,v tha u,s,v)from tensorflow and when comput_uv is false get handled in the tests. Now only dtype mismatches happens
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.svd
2 participants