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

Is there a bug in the OHEMSampler? #3677

Merged
merged 5 commits into from
Sep 9, 2020
Merged

Is there a bug in the OHEMSampler? #3677

merged 5 commits into from
Sep 9, 2020

Conversation

yuzhj
Copy link
Contributor

@yuzhj yuzhj commented Sep 3, 2020

I got the error(TypeError: forward() missing 1 required positional argument: 'x_reg') when i used the OHEMSampler in the DoublehandRCNN, so I checkd the ohem_sampler.py and double_roi_head.py.
line 27(double_roi_head.py) :
cls_score, bbox_pred = self.bbox_head(bbox_cls_feats, bbox_reg_feats)
line 37(ohem_sampler.py):
cls_score, _ = self.bbox_head(bbox_feats)
the arguments are different.
in the meantime, self.with_shared_head is in the double_roi_head.py but not in the ohem_sampler.py
so I did some fixes

fix the bug in the ohem_sampler
@CLAassistant
Copy link

CLAassistant commented Sep 3, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #3677 into master will decrease coverage by 0.04%.
The diff coverage is 48.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3677      +/-   ##
==========================================
- Coverage   61.10%   61.06%   -0.05%     
==========================================
  Files         216      216              
  Lines       15222    15318      +96     
  Branches     2530     2608      +78     
==========================================
+ Hits         9302     9354      +52     
- Misses       5458     5498      +40     
- Partials      462      466       +4     
Flag Coverage Δ
#unittests 61.06% <48.19%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
mmdet/apis/inference.py 20.73% <0.00%> (ø)
mmdet/apis/test.py 13.08% <0.00%> (+0.47%) ⬆️
mmdet/datasets/coco.py 45.39% <0.00%> (ø)
mmdet/models/detectors/fast_rcnn.py 43.75% <ø> (+4.86%) ⬆️
mmdet/models/roi_heads/point_rend_roi_head.py 21.81% <0.00%> (-2.93%) ⬇️
mmdet/models/roi_heads/htc_roi_head.py 31.83% <2.32%> (-2.63%) ⬇️
mmdet/core/bbox/samplers/ohem_sampler.py 83.78% <50.00%> (-2.33%) ⬇️
mmdet/models/detectors/rpn.py 59.37% <50.00%> (ø)
mmdet/models/roi_heads/grid_roi_head.py 66.66% <57.14%> (+2.53%) ⬆️
mmdet/models/roi_heads/cascade_roi_head.py 72.13% <61.70%> (-1.96%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update feb703e...110a4ef. Read the comment docs.

@hellock hellock requested a review from yhcao6 September 3, 2020 07:13
self.bbox_roi_extractor = context.bbox_roi_extractor[
context.current_stage]
self.bbox_head = context.bbox_head[context.current_stage]
self.bbox_head = self.context.bbox_head[self.context.current_stage]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is something wrong with the OHEM usage in cascade rcnn. May I ask if you would like to fix that? If not, you can add an assertion here and I will fix it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is something wrong with the OHEM usage in cascade rcnn. May I ask if you would like to fix that? If not, you can add an assertion here and I will fix it soon.

Yes, I'd like to fix that, but I only have time on weekends.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem

I fixed the OHEM usage problem in cascade rcnn,please have a check

@yuzhj yuzhj requested a review from yhcao6 September 6, 2020 16:55
@hellock hellock merged commit 5fffdda into open-mmlab:master Sep 9, 2020
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.

None yet

4 participants