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 37 commits into
base: main
Choose a base branch
from

Conversation

Daniel4078
Copy link
Contributor

@Daniel4078 Daniel4078 commented Jun 20, 2024

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

Jin Wang and others added 3 commits July 7, 2024 14:37
… 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
…timeError: Can't call numpy() on Tensor that requires grad. Use tensor.detach().numpy() instead." when the test function calls np.asarray to the returned value
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.

The reason the test is failing is you're trying to convert a torch tensor to a numpy array, without detaching the gradients (using tensor.detach().numpy()). I don't really understand the reasoning behind changing the format of the tests, such as the comment I left below - can't we just change the atol_/rtol_ values with test_frontend_function, rather than adding these extra assertions? Or are there other reasons you did this?

@Daniel4078
Copy link
Contributor Author

The reason the test is failing is you're trying to convert a torch tensor to a numpy array, without detaching the gradients (using tensor.detach().numpy()). I don't really understand the reasoning behind changing the format of the tests, such as the comment I left below - can't we just change the atol_/rtol_ values with test_frontend_function, rather than adding these extra assertions? Or are there other reasons you did this?

But the part that convert a torch tensor to a numpy array is not implemented by me and the same process works for tests of tenserflow.linalg.svd so I did not think I should not do the convertion. Or is there some way to do the same computation without converting? Thank you!

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 like these svd tests are still failing, could you try to resolve these as well? But otherwise that makes sense 👍 thanks! 😊

  • ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py::test_torch_svd
  • ivy_tests/test_ivy/test_frontends/test_numpy/test_linalg/test_decompositions.py::test_numpy_svd
  • ivy_tests/test_ivy/test_frontends/test_jax/test_numpy/test_linalg.py::test_jax_svd
  • ivy_tests/test_ivy/test_frontends/test_jax/test_lax/test_linalg.py::test_jax_svd
  • ivy_tests/test_ivy/test_frontends/test_tensorflow/test_raw_ops.py::test_tensorflow_Svd

Jin Wang added 5 commits July 16, 2024 08:31
…em is that the ground truth returns a ivy array with dtype the same as input, while the function returns a torch tensor with the torch version of that dtype.
…ubset_by_index parameter. tensorflow,torch and paddle backends are all failing due to significant value differences in only part of the reconstructed matrixes.
….svd produces significant difference with compute_uv=True (like flipped signs) in the reconstructed matrixes for all backends. I suspect that the decomposition of jax.lax.linalg.svd is different from other similiar functions
@Daniel4078
Copy link
Contributor Author

it is strange that with the same calculation to reconstruct the decomposited matrix (used in all other relevant tests), current jax.numpy.linalg.svd passes all tests while current jax.lax.linalg.svd fails with significantly different values produced. May be the decomposition of jax.lax.linalg.svd is different from the common way?

Daniel4078 and others added 14 commits July 18, 2024 21:18
…ecompositions.py::test_numpy_svd, though seems like using ivy.astype to convert type does not work at all
….py::test_tensorflow_Svd, now the frontend somehow always return float64 nomatter input dtype
….py::test_tensorflow_Svd, though if input is complex number the results show large difference
…pack_ops.py::test_torch_svd so that complex number inputs are also tested, they are passing
…turn fixed dtypes depending on if input is complex,
…alue instead as the same as the input, but the documents shows that the S component will always be real, which is different from the groundtruth behavior shown in the, updated the jax svd tests to check for complex inputs
….svd fail for tensorflow backend due to significant and updated all relavent svd with more complex supported dtype
@Daniel4078
Copy link
Contributor Author

found that the svd function of tenserflow backend does not support complex input, this may be the reason why some of the frontend tests for svd returns wrong results in tensorflow backend for complex value inputs. Also, I want to ask is the first component of the return of the helpers.test_frontend_function representing the groundtruth return?

1 similar comment
@Daniel4078
Copy link
Contributor Author

found that the svd function of tenserflow backend does not support complex input, this may be the reason why some of the frontend tests for svd returns wrong results in tensorflow backend for complex value inputs. Also, I want to ask is the first component of the return of the helpers.test_frontend_function representing the groundtruth return?

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