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

Implement FCOS training tricks #2935

Merged
merged 37 commits into from Jun 16, 2020
Merged

Conversation

Chrisfsj2051
Copy link
Contributor

Implement another three training tricks (except for center_sampling) used in the official repo (https://github.com/tianzhi0549/FCOS). By jointly using them, mAP gains of up to 4.9 (with DCN) and 1.3 (without DCN) will be brought.

In this implementation, the following results are achieved:
without DCN: mAP=38.6 (0.1 lower than the official repo)
with DCN: mAP=42.5 (0.2 higher than the official repo)

Train FCOS with four tricks (including center_sampling) arrording to official repo (https://github.com/tianzhi0549/FCOS).

without DCN:  mAP=38.6 (0.1 lower than official repo)
with DCN     :  mAP=42.5 (0.2 higher than official repo)
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
norm_cfg=self.norm_cfg,
bias=self.norm_cfg is None))
bias=True))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Originally most of the time bias will be False since FCOS uses GN in the head. Could I ask if bias has a great effect on the performance?

@yhcao6
Copy link
Collaborator

yhcao6 commented Jun 8, 2020

Thanks for your contribution, please check the comments!

Copy link
Collaborator

@Johnson-Wang Johnson-Wang left a comment

Choose a reason for hiding this comment

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

Please also load the original checkpoint to see if it is compatible.

mmdet/models/dense_heads/fcos_head.py Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Show resolved Hide resolved
norm_cfg=self.norm_cfg,
bias=self.norm_cfg is None))
bias=True))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is bias=True used in the original implementation? Actually, using bias before a normalization layer is not so standard, it's better to enable users to specify it.

@Chrisfsj2051
Copy link
Contributor Author

Chrisfsj2051 commented Jun 8, 2020

Hi @yhcao6 and @Johnson-Wang , thanks for your comments!

The origin setting and implementation of FCOS in mmdetection are slightly different from official repo. For example, the author set nms_th as 0.6 and doesn't use grad clip, while in mmdetection the nms_th is set as 0.5 and grad clip is used. Also, the official repo use bias=True in conv layer while the previous implementation in mmdetection doesn't. Here is the link to the official implementation of FCOSHead.

I implement and train this model by following the author, which may lead to differences from the previous implementation in mmdetection. I suggest to keep the bias setting for now. Also, I will conduct some experiments to see if it will raise significant difference, which will take about two days.

@Johnson-Wang
Copy link
Collaborator

Please check your ci test.

cls_scores = []
bbox_preds = []
centernesses = []
for i, x in enumerate(feats):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since there is no special operation on a specific fpn layer, is it possible to revert to forward_single implementation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Except that there would be another argument (stride) to be passed to forward_single.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Already updated in the latest commit.

mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Johnson-Wang Johnson-Wang left a comment

Choose a reason for hiding this comment

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

The main implementation seems fine to me now. There is another work to do here. The unittest of fcos head was missing. It would be better to add unittest gradually in the future. Could you please add the unittest of fcos? (Special attention should be paid to corner cases such as bbox_ignore, empty gt_bbox et al. )

mmdet/models/dense_heads/fcos_head.py Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
@Chrisfsj2051
Copy link
Contributor Author

Hi @Chrisfsj2051, it's great to see the implementation of improved FCOS. I was interested the performance of FCOS if you just add the dconv2 layer to the last conv layers without using the dconv backbone. Thanks.

Sure, I will try this later.

Cool! Thank you.

Get mAP=39.9 without using DCN in backbone.

@hyz-xmaster
Copy link
Contributor

Hi @Chrisfsj2051, it's great to see the implementation of improved FCOS. I was interested the performance of FCOS if you just add the dconv2 layer to the last conv layers without using the dconv backbone. Thanks.

Sure, I will try this later.

Cool! Thank you.

Get mAP=39.9 without using DCN in backbone.

Amazing. Thanks a lot.

# random inputs
gt_bboxes = [
torch.Tensor([[23.6667, 23.8757, 238.6326, 151.8874]]),
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it support gt_bboxes_ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unit test doesn't include gt_bboxes_ignore test, since the implemenration of FCOS doesn't support gt_bboxes_ignore (passed as argument but not actually used).

mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
mmdet/models/dense_heads/fcos_head.py Outdated Show resolved Hide resolved
@Johnson-Wang
Copy link
Collaborator

Still, please have a check on the ci.

@@ -80,26 +126,31 @@ def _init_layers(self):
self.reg_convs = nn.ModuleList()
for i in range(self.stacked_convs):
chn = self.in_channels if i == 0 else self.feat_channels
if self.dcn_on_last_conv and \
i == self.stacked_convs - 1:
Copy link
Member

@hellock hellock Jun 16, 2020

Choose a reason for hiding this comment

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

Use parentheses instead of backslashes.
if (self.dcn_on_last_conv and i == self.stacked_convs - 1) and let yapf to format it.

centerness_on_reg (bool): If true, position centerness on the
regress branch. Please refer to https://github.com/tianzhi0549/FCOS/issues/89#issuecomment-516877042.
dcn_on_last_conv (bool): If true, use dcn in the last layer of
towers.
Copy link
Member

Choose a reason for hiding this comment

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

Default values of several arguments are not described.

@Chrisfsj2051
Copy link
Contributor Author

Hi @hellock , thanks for the comments!
Default values for some arguments have been specified currently. However, I didn't describe the other default values (regress_ranges, loss_cls, etc.) since they might be too long to be properly shown in docstring.

@hellock hellock merged commit 565e070 into open-mmlab:master Jun 16, 2020
mike112223 pushed a commit to mike112223/mmdetection that referenced this pull request Aug 25, 2020
* 123

* implement FCOS with tricks (FCOS_plus)

Train FCOS with four tricks (including center_sampling) arrording to official repo (https://github.com/tianzhi0549/FCOS).

without DCN:  mAP=38.6 (0.1 lower than official repo)
with DCN     :  mAP=42.5 (0.2 higher than official repo)

* fix

* fix

* reformatted

* fix configs & reformat

* add docstring

* Update fcos_center-normbbox-centeronreg-giou_r50_caffe_fpn_gn-head_dcn_4x4_1x_coco.py

* add noqa: E501 since the url is too long

* fix flake

* fix flake

* fix yapf

* fix

* rewrite forward & reformat

* reformat & add docstring

* reformat

* reformat

* update docstring

* TODO: add unit test

Should be finished tomorrow

* add unit test

* fix

* fix docstring

* reformat

* add docstring

* reformat

* Update fcos_head.py

* Update fcos_head.py

* Update fcos_head.py

* Update fcos_head.py

* Update fcos_head.py

* Update test_heads.py

* Update fcos_head.py

* Update README.md
@hedes1992
Copy link

hedes1992 commented Sep 25, 2020

Hello,
Thank you very much for your great work.
I just find some bugs(maybe) in the implementation which will not disturb the training process, but just the in-consistency...
In detail, in fcos_head.py, the distance2bbox function just calculate the predicting bbox and gt bbox in the original image. But if self.norm_on_bbox is True, like here,
the "pos_bbox_targets" is normed by corresponding stride, but the "pos_points" is in the original image.
In this way, the "pos_decoded_target_preds" is not the true gt bboxes... But this will not cause error, because the loss_bbox here is IoULoss(which just use the relative offset between pred and gt to calculate loss).
Is my worry is correct?

FANGAreNotGnu pushed a commit to FANGAreNotGnu/mmdetection that referenced this pull request Oct 23, 2023
Co-authored-by: Tony Hu <tonyhu@amazon.com>
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

6 participants