Completed Max_unpool1d implementation

[mohame54]

PR Description

Completed max_unpool1d function implementation I didn't add paddle backend implementation cuz paddle.max_unpool1d had some errors related paddle itself

[github-actions]

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

[ivy-leaves]

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you 🤗

[mohame54]

my implementation is close to torch 2.0.1+cu118 version I don't know why gpu version of torch.max_unpool1d is different from the cpu version @AnnaTz

[AnnaTz]

my implementation is close to torch 2.0.1+cu118 version I don't know why gpu version of torch.max_unpool1d is different from the cpu version @AnnaTz

I get this error when I run the test locally:

E       AssertionError:  the results from backend torch and ground truth framework jax do not match
E        [[[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]]
E       
E        [[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]]
E       
E        [[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1.  0.  1.  0.]]]!=[[[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]]
E       
E        [[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]]
E       
E        [[-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1. -1. -1.  0.]
E         [-1.  0.  2.  0.]]] 
E       
E       
E       Falsifying example: test_max_unpool1d(
E           backend_fw='torch',
E           on_device='cpu',
E           x_k_s_p=(['float32', 'int64'], array([[[-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1., -1., -1.]],
E            
E                   [[-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1., -1., -1.]],
E            
E                   [[-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1., -1., -1.],
E                    [-1.,  1.,  2.]]], dtype=float32), array([[[0, 1, 2],
E                    [0, 1, 2],
E                    [0, 1, 2],
E                    [0, 1, 2]],
E            
E                   [[0, 1, 2],
E                    [0, 1, 2],
E                    [0, 1, 2],
E                    [0, 1, 2]],
E            
E                   [[0, 1, 2],
E                    [0, 1, 2],
E                    [0, 1, 2],
E                    [0, 2, 2]]]), (2,), (1,), 0),

It seems the failure happens when running on cpu, so I don't understand what you mean exactly.
Is there a hands-on example that shows the original torch function is inconsistent across versions?

[mohame54]

Yeah, I will show you one example.

[mohame54]

[mohame54]

you can see here that's torch cuda max_unpool output from the same input above which is different from the cpu version @AnnaTz

[AnnaTz]

Ohh I get what's happening 😅
If you run the specific example many times it will sometimes return the correct value. The result is non-deterministic because the indices contain repeated values.
They actually mention it in their documentation

[mohame54]

Yeah, that's a catch I tried before to remove the repeated indices but it was not useful also the tests were giving me errors

[AnnaTz]

Yeah, that's a catch I tried before to remove the repeated indices but it was not useful also the tests were giving me errors

What kind of errors? I think you should be able to force the indices to not include identical values. We should at least know if that's the only problem (if the test passes for all other cases) before we decide what to do about it.

[mohame54]

output mismatch errors with torch backend if tried to exclude the repeated indices, as you can see so I didn't add the excluding part to the implementation .

[mohame54]

What kind of errors? I think you should be able to force the indices to not include identical values. We should at least know if
that's the only problem (if the test passes for all other cases) before we decide what to do about it.

I tried as many tests as I could but the only errors that appeared to me were output mismatching errors with of course torch backend .

[mohame54]

Also I didn't complete the paddle backend implementation cuz the paddle function was giving me errors also and when I tried it on colab with paddle 2.4.0 installed it didn't work for me. it seems that the function has not been implemented yet.

[AnnaTz]

I'm inclined to say we will need a compositional implementation for this 💀

[mohame54]

->I'm inclined to say we will need a compositional implementation for this 💀
but Why?

[mohame54]

Do you mean compositional implementation using ivy API instead of backend specific API, wouldn't that affect the performance 😕

[AnnaTz]

->I'm inclined to say we will need a compositional implementation for this 💀 but Why?

We need the ivy function to work consistently for all backends in all cases. The native torch function does not work as expected in the repeated indices case, and the paddle function is not implemented in some versions and giving errors in others. So, we need an ivy function implementation to cover these.

[AnnaTz]

Do you mean compositional implementation using ivy API instead of backend specific API, wouldn't that affect the performance 😕

Also, about that, according to what @vedpatwardhan has told me, I think the implementation we have for the numpy, jax and tensorflow backends could be replaced by an ivy composition. It seems that we are doing the exact same operations in all these backends, so it would be better to avoid code repetition.

[mohame54]

Also, about that, according to what @vedpatwardhan has told me, I think the implementation we have for the numpy, jax and
tensorflow backends could be replaced by an ivy composition. It seems that we are doing the exact same operations in all >these backends, so it would be better to avoid code repetition.

if we want we can ignore the torch max_unpool1d backend implementation and consider custom torch implementation like jax and numpy case.

[mohame54]

But I agree with you in order to reduce the code repetition we need compositional implementation

[AnnaTz]

Also, about that, according to what @vedpatwardhan has told me, I think the implementation we have for the numpy, jax and
tensorflow backends could be replaced by an ivy composition. It seems that we are doing the exact same operations in all >these backends, so it would be better to avoid code repetition.

if we want we can ignore the torch max_unpool1d backend implementation and consider custom torch implementation like jax and numpy implementations.

We should not completely ignore the native torch function, because in the cases it does work it will be faster than a composition. Hence, we need to convert ivy.max_unpool1d into a partial mixed function.

[AnnaTz]

I think it will be quite simple since you have already implemented the backend functions compositionally. I think you will mainly copy that and replace native calls with ivy calls.

[mohame54]

sorry I didn't mean to edit your comment so sorry @AnnaTz 😄

[mohame54]

it's done @AnnaTz

[AnnaTz]

Also, please remove the docs/demos changes.

[mohame54]

If I remove them there will be conflicts with the main branch

[AnnaTz]

@sherry30 are we allowed to use native functions in testing?

[AnnaTz]

The code looks good overall. The only thing remaining is to add the partial torch backend.

[mohame54]

What do you mean by partial torch backend? I did not get what you meant here

[AnnaTz]

https://unify.ai/docs/ivy/overview/deep_dive/function_types.html#partial-mixed-functions