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.

Sign up for GitHub

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

Jump to bottom

[Feature] Support QueryInst #6050

Merged
merged 25 commits into from
Oct 26, 2021
Merged

[Feature] Support QueryInst #6050

merged 25 commits into from
Oct 26, 2021

Conversation

vealocia
Copy link
Contributor

@vealocia vealocia commented Sep 7, 2021 •
edited by jshilong
Loading

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

Involve the newly proposed instance segmentation method QueryInst into mmdetection

Modification

Please briefly describe what modification is made in this PR.

BC-breaking

attn_feats is added to the returns of mmdet/models/roi_heads/bbox_heads/dii_head.py

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

  • Pre-commit or other linting tools are used to fix the potential lint issues.
  • The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
  • The documentation has been modified accordingly, like docstring or example tutorials.

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

ZwwWayne
ZwwWayne reviewed Sep 7, 2021
View reviewed changes
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Sep 7, 2021

Hi @vealocia ,
Thanks for the efforts. QueryInst wil be a good feature for MMDetection and we plan to merge this PR in the following days (hopefully the next release in Sept.). Would you like to add unit tests to test DiceLoss and SparseRoIHead following those tests in the ./tests directory?

@vealocia
Copy link
Contributor Author

vealocia commented Sep 7, 2021

Hi @vealocia ,
Thanks for the efforts. QueryInst wil be a good feature for MMDetection and we plan to merge this PR in the following days (hopefully the next release in Sept.). Would you like to add unit tests to test DiceLoss and SparseRoIHead following those tests in the ./tests directory?

Hi @ZwwWayne . No problem, I will finish it as soon as possible.

@vealocia
Copy link
Contributor Author

vealocia commented Sep 7, 2021

Hi @ZwwWayne , I implement unit tests for DiceLoss and DynamicMaskHead. Please take a look, many thanks~🙂

@RangiLyu RangiLyu mentioned this pull request Sep 7, 2021
Closed
20 tasks
hhaAndroid
hhaAndroid reviewed Sep 10, 2021
View reviewed changes
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmdet/models/losses/dice_loss.py Outdated Show resolved Hide resolved
mmdet/models/roi_heads/bbox_heads/dii_head.py Outdated Show resolved Hide resolved
mmdet/models/roi_heads/mask_heads/dynamic_mask_head.py Outdated Show resolved Hide resolved
mmdet/models/roi_heads/mask_heads/dynamic_mask_head.py Outdated Show resolved Hide resolved
mmdet/models/roi_heads/mask_heads/dynamic_mask_head.py Show resolved Hide resolved
mmdet/models/roi_heads/sparse_roi_head.py Show resolved Hide resolved
mmdet/models/utils/transformer.py Show resolved Hide resolved
@ZwwWayne
Copy link
Collaborator

Suggest merging master to use the CI/CD.

@vealocia
Copy link
Contributor Author

Suggest merging master to use the CI/CD.

The latest master branch has been merged.

@Yuxin-CV Yuxin-CV mentioned this pull request Sep 15, 2021
Closed
jshilong
jshilong reviewed Oct 22, 2021
View reviewed changes
Copy link
Collaborator

@jshilong jshilong left a comment

Choose a reason for hiding this comment

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

The overall design LGTM, I believe it can be merged after all left conversations are resolved.
@ZwwWayne

@vealocia
Copy link
Contributor Author

@jshilong Your concerns are carefully resolved now!

@jshilong
Copy link
Collaborator

jshilong commented Oct 22, 2021 •
edited
Loading

@ZwwWayne Maybe we can merge this feature to this or the next version.

@jshilong
Copy link
Collaborator

jshilong commented Oct 22, 2021 •
edited
Loading

@jshilong Your concerns are carefully resolved now!

@vealocia Could you benchmark this version and provide corresponding checkpoints and log files via google driver?
So we can create a readme like
https://github.com/open-mmlab/mmdetection/blob/master/configs/atss/README.md

Many thanks again for your excellent works.

@BIGWangYuDong Could you help to process and rename checkpoints from @vealocia and update the URL to the readme?

New Algorithms automation moved this from Review in progress to Reviewer approved Oct 22, 2021
@vealocia
Copy link
Contributor Author

@jshilong Your concerns are carefully resolved now!

@vealocia Could you benchmark this version and provide corresponding checkpoints and log files via google driver? So we can create a readme like https://github.com/open-mmlab/mmdetection/blob/master/configs/atss/README.md

Many thanks again for your excellent works.

@BIGWangYuDong Could you help to process and rename checkpoints from @vealocia and update the URL to the readme?

I will push my prepared README.md and leave the download link to a placeholder. Thanks for your collaboration too and glad to work with you ;)

@vealocia
Copy link
Contributor Author

Hi @vealocia ,
Thanks for the efforts. It is highly possible that we can make it in this month! Would you also like to add the configs/queryinst/README.md to add the bibetex and the performance of QueryInst just like the other methods? You may also update QueryInst in the model zoo and README.md pages.

Thanks for the cooperations from all your maintainers too. All pretrained models and logs are prepared and I will update the performance and models asap. But I don't know how to upload these models and logs to your custom server (download.openmmlab.com), can you help me?

Hi @vealocia , Would you like to put them to google drive or BaiduYUN for us to download and test the model?

The share link to queryinst checkpoints is https://pan.baidu.com/s/1TVuusygUX1M6YXEIwWR9tQ passwork: hbk0

QueryInst pretrained checkpoints can be downloaded here ⬆️

@vealocia
Copy link
Contributor Author

I just push the README.md and meta.yaml with leave all link of checkpoints to placeholder.

@jshilong
Copy link
Collaborator

I just push the README.md and meta.yaml with leave all link of checkpoints to placeholder.

@ZwwWayne @BIGWangYuDong

@ZwwWayne ZwwWayne merged commit f003656 into open-mmlab:master Oct 26, 2021
New Algorithms automation moved this from Reviewer approved to Done Oct 26, 2021
@ljpadam ljpadam mentioned this pull request Nov 3, 2021
Closed
ZwwWayne pushed a commit to ZwwWayne/mmdetection that referenced this pull request Jul 19, 2022
@vealocia @ZwwWayne
[Feature] Support QueryInst (open-mmlab#6050)
097abdd 
* impl queryinst

* bug free queryinst with crop and negative samples

* use detr hyperparameters

* pre-commit hooks

* modified dynamic_mask_head docstrings

* remove unused dropout in dynamic_mask_head

* add docstring for dice_loss

* add dice_loss unit test

* impl unit test for dynamic_mask_head

* update queryinst docstring and implementation

* stability update for dice_loss and dynamic_mask_head

* update for clarify

* bug free in case of num_proposals equal to zero

* detail docstrings

* fixed CI issues

* issues resolved

* add queryinst docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kyuuki93 Kyuuki93 Kyuuki93 left review comments

@ZwwWayne ZwwWayne ZwwWayne approved these changes

@jshilong jshilong jshilong approved these changes

@AronLin AronLin Awaiting requested review from AronLin

@hhaAndroid hhaAndroid Awaiting requested review from hhaAndroid

@BIGWangYuDong BIGWangYuDong Awaiting requested review from BIGWangYuDong

Assignees
No one assigned
Labels
None yet
Projects
New Algorithms
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vealocia @CLAassistant @ZwwWayne @hhaAndroid @jshilong @Kyuuki93 @BIGWangYuDong