frontends.torch.Tensor: add matmul instance method #6935
Conversation
|
Hi,
Just thought you should know. |
| ) | ||
| def test_torch_instance_matmul( | ||
| dtype_tensor1_tensor2, | ||
| as_variable, |
There was a problem hiding this comment.
where are as_variable and native_array defined?
There was a problem hiding this comment.
The test_torch_instance_matmul error stack trace reads:
_ test_torch_instance_matmul[cpu-ivy.functional.backends.tensorflow-False-False-tensorflow] _
get_command_line_flags = {'as_variable': None, 'container': None, 'instance_method': None, 'native_array': None, ...}
device = 'cpu'
f = <module 'ivy.functional.backends.tensorflow' from '/ivy/ivy/functional/backends/tensorflow/__init__.py'>
fw = 'tensorflow', fixt_frontend_str = 'torch', args = (), kwargs = {}
@given(data=st.data())
> @settings(max_examples=1)
ivy_tests/test_ivy/helpers/testing_helpers.py:184:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
ivy_tests/test_ivy/helpers/testing_helpers.py:226: in new_fn
return test_fn(*args, **kwargs)
ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py:2582: in test_torch_instance_matmul
@given(
ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py:2591: in test_torch_instance_matmul
helpers.test_frontend_method(
ivy_tests/test_ivy/helpers/function_testing.py:1429: in test_frontend_method
value_test(
ivy_tests/test_ivy/helpers/assertions.py:122: in value_test
assert_all_close(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
ret_np = array([[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[24.125, 12. , 12. ],
[38. , 18.125, 18.125]]])
ret_from_gt_np = array([[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[24.25, 12. , 12. ],
[38.5 , 18.25, 18.25]]])
rtol = 0.01, atol = 1e-06, ground_truth_backend = 'torch'
There was a problem hiding this comment.
The @handle_cmd_line_args decorator takes care of these strategies at runtime so there's no need to explicitly define them in the test. I have never had to define them previously whenever I have contributed to the codebase. Additionally, you can read point no. 3 here. One of the maintainers of this repo actually advised me against defining strategies for as_variable and native_array and let the @handle_cmd_line_args take care of that for us. I don't think anyone else has defined them explicitly either.
There was a problem hiding this comment.
Hi @hmahmood24, indeed you're correct, my apologies, I haven't been working on the frontends so I'm a little behind with some of the strategies updates.
As for the the assertion error, perhaps try adjusting the rtol and see if that makes a difference, although I'm concerned about the tolerance being high already. This issue is surprising, especially given the ground truth reference is torch also.
|
Hi there, thanks for the contribution! And thanks for letting me know about this. Please see me comments regarding the test you've written. |
| ) | ||
| def test_torch_instance_matmul( | ||
| dtype_tensor1_tensor2, | ||
| as_variable, |
There was a problem hiding this comment.
The test_torch_instance_matmul error stack trace reads:
_ test_torch_instance_matmul[cpu-ivy.functional.backends.tensorflow-False-False-tensorflow] _
get_command_line_flags = {'as_variable': None, 'container': None, 'instance_method': None, 'native_array': None, ...}
device = 'cpu'
f = <module 'ivy.functional.backends.tensorflow' from '/ivy/ivy/functional/backends/tensorflow/__init__.py'>
fw = 'tensorflow', fixt_frontend_str = 'torch', args = (), kwargs = {}
@given(data=st.data())
> @settings(max_examples=1)
ivy_tests/test_ivy/helpers/testing_helpers.py:184:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
ivy_tests/test_ivy/helpers/testing_helpers.py:226: in new_fn
return test_fn(*args, **kwargs)
ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py:2582: in test_torch_instance_matmul
@given(
ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py:2591: in test_torch_instance_matmul
helpers.test_frontend_method(
ivy_tests/test_ivy/helpers/function_testing.py:1429: in test_frontend_method
value_test(
ivy_tests/test_ivy/helpers/assertions.py:122: in value_test
assert_all_close(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
ret_np = array([[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[24.125, 12. , 12. ],
[38. , 18.125, 18.125]]])
ret_from_gt_np = array([[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[12. , 12. , 12. ],
[12. , 12. , 12. ]],
[[24.25, 12. , 12. ],
[38.5 , 18.25, 18.25]]])
rtol = 0.01, atol = 1e-06, ground_truth_backend = 'torch'
| ) | ||
| def test_torch_instance_matmul( | ||
| dtype_tensor1_tensor2, | ||
| as_variable, |
There was a problem hiding this comment.
Hi @hmahmood24, indeed you're correct, my apologies, I haven't been working on the frontends so I'm a little behind with some of the strategies updates.
As for the the assertion error, perhaps try adjusting the rtol and see if that makes a difference, although I'm concerned about the tolerance being high already. This issue is surprising, especially given the ground truth reference is torch also.
| # matmul | ||
| @handle_cmd_line_args | ||
| @given( | ||
| dtype_tensor1_tensor2=_get_dtype_and_multiplicative_matrices().example(), |
There was a problem hiding this comment.
.example() should not be called in the data generation. Perhaps you kept this in whilst testing?
There was a problem hiding this comment.
Hi @simonetgordon, yeah, my bad. I forgot to remove .example() from the test itself before committing the code. I have removed that, as well as rtol itself and tried fixing the tests. They run perfectly fine on my machine against all backends as of the latest commit. Looking forward to your feedback now.
There was a problem hiding this comment.
Hold up.
Someone refactored the entire testing criteria a few hours ago and made major changes to the frontend tests. I took the latest pull to merge any conflicts with my latest commits but when I try to run my tests, hypothesis fails throwing all sorts of weird errors. It isn't even able to collect the tests, running them is a different story altogether. I think I will wait out till these new changes are made stable enough and the errors are fixed on the main branch before running tests specific to just my function.
@simonetgordon can you please have someone look into the ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py file so they can fix these errors as a consequence of the recent refactoring? Thanks!
There was a problem hiding this comment.
Hi @hmahmood24, yes I am aware of the recent frontend decorator changes, apologies for the inconvenience! I will let you know when they've been fixed.
Since you said the tests worked fine for you locally, hopefully we only need to wait for the decorator issues to be resolved to get this merged. If it takes too long, I will merge this PR within a reasonable amount of time.
|
|
||
|
|
||
| # matmul | ||
| @handle_cmd_line_args |
There was a problem hiding this comment.
we now have 4 new decorators instead of handle_cmd_line_args :
handle_function: used for Ivy functional APIhandle_method: used for Ivy methodshandle_frontend_function: used for frontend functionshandle_frontend_method: used for frontend methods, methods tests are now skipped temporarily.
The last decorator is the one you'll need to replace handle_cmd_line_args with. Unfortunately, this is the only one currently not functioning correctly, but it will be rectified soon!
There was a problem hiding this comment.
@hmahmood24 perhaps you didn't see this update?
There was a problem hiding this comment.
Apologies I wasn't able to reply earlier. I have seen the updates and I have also refactored my tests in coherence with these updates but to no avail. The test_tensor.py file has still so many bugs that prevents the tests from getting collected. At this point, I'm not sure what's the best course of action for me apart from just waiting this out.
Have you asked anyone from the team to look into these bugs by now?
There was a problem hiding this comment.
@hmahmood24 Hi there, yes someone is trying to tackle these but there should have already been some fixes pushed.
Also, I don't see the new decorator here in the PR version. Perhaps you forgot to push?
There was a problem hiding this comment.
@simonetgordon Hey, yes I didn't push my changes since the tests weren't collecting locally. I have made the changes and pushed them to remain coherent with all the other tests now.
There was a problem hiding this comment.
Hi @simonetgordon,
Any updates since it’s been almost 2 weeks since this PR got opened? Anything from my end to add here?
|
|
||
|
|
||
| # matmul | ||
| @handle_cmd_line_args |
There was a problem hiding this comment.
@hmahmood24 Hi there, yes someone is trying to tackle these but there should have already been some fixes pushed.
Also, I don't see the new decorator here in the PR version. Perhaps you forgot to push?
Close #6557