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

Add torch_meshgrid_ij wrapper due to PyTorch change #2044

Merged
merged 5 commits into from
Jun 15, 2022

Conversation

pallgeuer
Copy link
Contributor

@pallgeuer pallgeuer commented Jun 7, 2022

Motivation

PyTorch is in the process of changing the behaviour of torch.meshgrid, and this results in a warning when using this method without an indexing argument as of PyTorch 1.10. The problem is that prior to PyTorch 1.10 these is no such argument, and thus for compatibility reasons it can't just be added. PyTorch 1.8.2 is an LTS release, so this issue will likely not go away any time soon, and eventually PyTorch will make a breaking change that changes the default behaviour of the method when called without an indexing argument:
https://pytorch.org/docs/stable/generated/torch.meshgrid.html
pytorch/pytorch#50276

While mmcv has no calls to torch.meshgrid, many other repos do (this is not an exhaustive list):
open-mmlab/mmpretrain#860
open-mmlab/mmdetection#8090
open-mmlab/mmtracking#577
open-mmlab/mmpose#1402

Thus, it would be best if mmcv could store a wrapper for torch_meshgrid_ij that deals with this difficulty, so that eventually all the other repos can just use the wrapper in mmcv instead of all defining their own or otherwise dealing with the issue.

Modification

This PR adds a suitable torch_meshgrid_ij wrapper.

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • CLA has been signed and all committers have signed the CLA in this PR.

@CLAassistant
Copy link

CLAassistant commented Jun 7, 2022

CLA assistant check
All committers have signed the CLA.

@pallgeuer
Copy link
Contributor Author

I have updated the PR based on the discussion here:
open-mmlab/mmdetection#8090 (comment)

mmcv/utils/torch_ops.py Outdated Show resolved Hide resolved
@zhouzaida
Copy link
Collaborator

Hi, thanks for your contribution. We can append this function at https://github.com/open-mmlab/mmcv/blob/master/mmcv/utils/__init__.py#L56.

Besides, it would be better to add unit tests for this function at https://github.com/open-mmlab/mmcv/tree/master/tests/test_utils/test_torch_ops.py.

@pallgeuer
Copy link
Contributor Author

Hi, in the process of trying to help you guys with this torch.meshgrid issue I have made multiple pull requests, and respectfully updated them for various wishes by the developers multiple times already. After already having sunk much time into this, I do not have the spare time to start working out and coding unit tests, and probably update them multiple times again before they can be accepted. I feel the whole process would from this point onwards be significantly more efficiently handled by the development team themselves.

@zhouzaida
Copy link
Collaborator

Hi, in the process of trying to help you guys with this torch.meshgrid issue I have made multiple pull requests, and respectfully updated them for various wishes by the developers multiple times already. After already having sunk much time into this, I do not have the spare time to start working out and coding unit tests, and probably update them multiple times again before they can be accepted. I feel the whole process would from this point onwards be significantly more efficiently handled by the development team themselves.

Thanks for your contributions. We will commit to this PR.

@zhouzaida zhouzaida requested a review from teamwong111 June 9, 2022 12:44
@zhouzaida zhouzaida requested a review from ZwwWayne June 15, 2022 06:44
@zhouzaida zhouzaida merged commit f5425ab into open-mmlab:master Jun 15, 2022
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.

4 participants