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 floordiv warning. #8648

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Fix floordiv warning. #8648

merged 2 commits into from
Aug 30, 2022

Conversation

JiayuXu0
Copy link
Contributor

Motivation

In SOLO code, floordiv is deprecated, and its behavior will change in a future version of pytorch.

Modification

torch.div(a, b, rounding_mode='trunc') is used to replace floordiv.

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2022

CLA assistant check
All committers have signed the CLA.

@RangiLyu
Copy link
Member

The unit tests of pytorch<=1.7 failed. MMDetection needs to support all versions of pytorch >= 1.6.

@JiayuXu0
Copy link
Contributor Author

If we add a conditional selection (torch version>=1.8 or torch version <1.8) before calculation, is it ok?

@ZwwWayne
Copy link
Collaborator

We can implement a floordiv wrapper that handles different pytorch versions, and use that wrapper uniformly in the head.

@ZwwWayne ZwwWayne added this to the 2.26.1 milestone Aug 28, 2022
@JiayuXu0
Copy link
Contributor Author

Thanks for your suggestion. I have modify the code and wait for your review.

@ZwwWayne ZwwWayne requested a review from RangiLyu August 29, 2022 07:18
@ZwwWayne ZwwWayne added the enhancement New feature or request label Aug 29, 2022
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #8648 (0931ee2) into dev (2f249ff) will increase coverage by 0.00%.
The diff coverage is 77.77%.

@@           Coverage Diff           @@
##              dev    #8648   +/-   ##
=======================================
  Coverage   64.07%   64.08%           
=======================================
  Files         361      361           
  Lines       29528    29536    +8     
  Branches     5021     5022    +1     
=======================================
+ Hits        18920    18927    +7     
- Misses       9591     9592    +1     
  Partials     1017     1017           
Flag Coverage Δ
unittests 64.08% <77.77%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmdet/utils/misc.py 62.22% <71.42%> (+0.68%) ⬆️
mmdet/models/dense_heads/solo_head.py 65.26% <100.00%> (+0.07%) ⬆️
mmdet/models/dense_heads/solov2_head.py 10.13% <100.00%> (+0.30%) ⬆️
mmdet/datasets/pipelines/transforms.py 75.80% <0.00%> (-0.17%) ⬇️
mmdet/core/post_processing/bbox_nms.py 79.10% <0.00%> (+4.47%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.



def floordiv(dividend, divisor, rounding_mode='trunc'):
if _torch_version_div_indexing:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, you can use torch.floor_divide directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

torch.floor_divide() is deprecated and will be removed in a future PyTorch release.
https://pytorch.org/docs/stable/generated/torch.floor_divide.html

@JiayuXu0 JiayuXu0 requested review from hhaAndroid and removed request for RangiLyu August 29, 2022 09:50
@ZwwWayne ZwwWayne merged commit df0694c into open-mmlab:dev Aug 30, 2022
@ZwwWayne
Copy link
Collaborator

We need to migrate the modification in MMDet 3.0

@JiayuXu0
Copy link
Contributor Author

We need to migrate the modification in MMDet 3.0

Should I summit a new pull request to branch dev-3.x? or it will be merged to 3.x automatically?

@BIGWangYuDong
Copy link
Collaborator

@JiayuXu0 Hi, you can checkout from dev-3.x and create a new PR to dev-3.x. Thanks for your PR!

ZwwWayne pushed a commit that referenced this pull request Sep 9, 2022
* Fix floordiv warning.

* Add floordiv wrapper.
SakiRinn pushed a commit to SakiRinn/mmdetection-locount that referenced this pull request Mar 17, 2023
* Fix floordiv warning.

* Add floordiv wrapper.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants