Add conv ops for mindspore frontend #20138
Conversation
There was a problem hiding this comment.
Hey,
Thanks for opening this PR!
You have only referenced an issue for Conv2d and not for the other implementations in this PR. We would need to close the other issues to avoid confusion! So maybe open issues for the other functions so that we can link them to this PR
Also, as part of this task, I think you are required to write tests for each of these functions.
Thanks
Thanks. I will write tests soon. |
|
@jkeane508 Sorry about the delay. I have written the tests. |
Yea I think so! Can you open issues for the other functions you've done in this PR. We can merge functions that haven't been linked as issues. Thanks! |
I have opened it. #20945 @jkeane508 |
Sorry, I misunderstanded. Here:
|
|
@jkeane508 |
Just reading about deformable conv2d. Is there a way to implement it using the source algorithm with functions that already exist in the Ivy functional API? |
|
I find it a little confusing as well. It seems that many deep learning backend frameworks support deformable convolution directly, such as MindSpore, PyTorch, and PaddlePaddle, making it efficient to implement. You can see some examples in the GitHub issue comment I provided (#20945 (comment)). TensorFlow doesn't have native support for it, despite several pull requests attempting to add it. So it is not our duty. For instance, PaddlePaddle provides documentation on how to use deformable convolution (https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/fluid/layers/deformable_conv_cn.html#paddle.fluid.layers.deformable_conv). While personal implementations are available for other frameworks like PyTorch (https://github.com/oeway/pytorch-deform-conv) and TensorFlow (https://github.com/kastnerkyle/deform-conv/), they can be complex and might encounter efficiency issues, especially when implemented in Python (I guess) due to the number of reshape-related operations involved. Given the complexity and lack of equivalent functions in Ivy, I suggest following the rule stated in the documentation: "In case a frontend function is complex and there is no equivalent Ivy function to use, it is strongly advised to add that function to our Experimental API." I have another concern about deformable convolution being currently required only for the MindSpore frontend, while other frameworks like PyTorch (torchvision) and PaddlePaddle have this API but haven't implemented it in the frontend. So, do we really need this? If you want, I can try to follow the implementation in this GitHub repository (https://github.com/kastnerkyle/deform-conv/), but I think that it might not make much sense. I hope this helps clarify the situation. Let me know if you have any further comment! @jkeane508 |
Yep, having looked at all the options. I think what makes the most sense is adding this to the Ivy Experimental API. Would you be keen to do this or shall I open an issue? |
|
I open one - [ivy.deform_conv2d] #21187 by Feature request. I can continue on it and it will take some time as it will be the first time for me to work on ivy functional api and backend api. Could you help me to link conv1d and conv3d to mindspore frontend to-do list (because they are not in the list)? And then we can merge this PR and close these three issues (conv1d, conv2d, conv3d). Thanks. @jkeane508 |
I am not sure this issue (#21187 ) is opened with correct labels, and I guess that it need to be reviewed. Many thanks if you can help me check it. |
Yea this looks good! |
|
Hey @xoiga123, Just wanted to request a review from you before merging if that's ok :) |
|
@xoiga123 pls review. Thanks. |
There was a problem hiding this comment.
Hey @themaigod, sorry for the super late reply 😅
The implementation and the reasoning all sound good, I only have some nitpicks about the tests.
Also, wouldn't it be more correct to place the tests into
test_mindspore/test_ops/test_function/test_mindspore_nn_func.py
instead of the current
test_mindspore/test_ops/test_mindspore_nn_func.py
I'm not sure why the file got created in the first place, as nn_func only exists in mindspore.ops.function.nn_func and not mindspore.ops.nn_func
| # | ||
| # # conv1d | ||
| # @handle_frontend_test( | ||
| # fn_tree="mindspore.ops.Conv1d", |
There was a problem hiding this comment.
This is a class, the correct functional path would be mindspore.ops.function.nn_func.conv1d
| # fn_tree="mindspore.ops.Conv1d", | ||
| # dtype_vals=x_and_filters(dim=1), | ||
| # ) | ||
| # def test_torch_conv1d( |
| # | ||
| # | ||
| # @handle_frontend_test( | ||
| # fn_tree="mindspore.ops.Conv2d", |
There was a problem hiding this comment.
mindspore.ops.function.nn_func.conv2d
| # fn_tree="mindspore.ops.Conv2d", | ||
| # dtype_vals=x_and_filters(dim=2), | ||
| # ) | ||
| # def test_torch_conv2d( |
| # | ||
| # | ||
| # @handle_frontend_test( | ||
| # fn_tree="mindspore.ops.Conv3d", |
There was a problem hiding this comment.
mindspore.ops.function.nn_func.conv3d
| # fn_tree="mindspore.ops.Conv3d", | ||
| # dtype_vals=x_and_filters(dim=3), | ||
| # ) | ||
| # def test_torch_conv3d( |
|
@xoiga123 Sorry for the mistakes in tests. I have fixed the name and the position issues. |
Using the implemented code in: ivy/functional/frontends/torch/nn/functional/convolution_functions.py
I also try to write deformable_conv2d. It seems that there is no ivy api for simple achieving that. So if we want to achieve it, we need to implement it based on the source algorithm. I am not sure it is the correct way.