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

DenseCL init weights copy query encoder weights to key encoder. #411

Merged
merged 2 commits into from Aug 19, 2022

Conversation

lorinczszabolcs
Copy link

@lorinczszabolcs lorinczszabolcs commented Aug 9, 2022

Motivation

DenseCL weight initialization is not done consistently with original repo, when loading ImageNet pre-trained weights to backbone, it will only get loaded to the query encoder, and not the key encoder, see original repo: https://github.com/WXinlong/DenseCL/blob/360df04ba25aca36d42e85c5a96552783568fccf/openselfsup/models/densecl.py#L36 and https://github.com/WXinlong/DenseCL/blob/360df04ba25aca36d42e85c5a96552783568fccf/openselfsup/models/densecl.py#L51-L58.

Fixes #404

Modification

DenseCL init includes calling super().init_weights() and then copying query encoder params to key encoder params.

Checklist

Before PR:

  • Pre-commit or other linting tools are used to fix the potential lint issues.
  • Bug fixes are fully covered by unit tests, the case that causes the bug should be added in the unit tests.
  • The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, like docstring or example tutorials.

After PR:

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

@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #411 (c56d4ad) into dev (171a475) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #411      +/-   ##
==========================================
+ Coverage   72.47%   72.48%   +0.01%     
==========================================
  Files         121      121              
  Lines        4759     4765       +6     
  Branches      763      763              
==========================================
+ Hits         3449     3454       +5     
  Misses       1151     1151              
- Partials      159      160       +1     
Flag Coverage Δ
unittests 72.48% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
mmselfsup/models/algorithms/densecl.py 100.00% <100.00%> (ø)
mmselfsup/utils/misc.py 97.05% <0.00%> (-2.95%) ⬇️

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

Copy link
Collaborator

@YuanLiuuuuuu YuanLiuuuuuu left a comment

Choose a reason for hiding this comment

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

LGTM

@lorinczszabolcs
Copy link
Author

One thing I noticed that is not optimal, is that the log info will still show that the key encoder weights did not change (due to it being printed in super().init_weights(), and only after that will its weights be initialized. I am not sure how that could be solved. At least the initialization is correct, but the log info would suggest otherwise. Any ideas?

@YuanLiuuuuuu
Copy link
Collaborator

YuanLiuuuuuu commented Aug 12, 2022

One thing I noticed that is not optimal, is that the log info will still show that the key encoder weights did not change (due to it being printed in super().init_weights(), and only after that will its weights be initialized. I am not sure how that could be solved. At least the initialization is correct, but the log info would suggest otherwise. Any ideas?

Currently, there is no way to overwrite the log for the the initialization log of the key encoder, unless the init_weight is modified in MMCV. But it's not practical at present. I think you can use another log here to indicate the key encoder is initialized by the query encoder, as a workaround. Thanks!

@lorinczszabolcs
Copy link
Author

One thing I noticed that is not optimal, is that the log info will still show that the key encoder weights did not change (due to it being printed in super().init_weights(), and only after that will its weights be initialized. I am not sure how that could be solved. At least the initialization is correct, but the log info would suggest otherwise. Any ideas?

Currently, there is no way to overwrite the log for the the initialization log of the key encoder, unless the init_weight is modified in MMCV. But it's not practical at present. I think you can use another log here to indicate the key encoder is initialized by the query encoder, as a workaround. Thanks!

Hi! I see. So would that be done by using mmselfsup.utils.get_root_logger to get a logger and then log with that, or just a simple print statement?

@YuanLiuuuuuu
Copy link
Collaborator

One thing I noticed that is not optimal, is that the log info will still show that the key encoder weights did not change (due to it being printed in super().init_weights(), and only after that will its weights be initialized. I am not sure how that could be solved. At least the initialization is correct, but the log info would suggest otherwise. Any ideas?

Currently, there is no way to overwrite the log for the the initialization log of the key encoder, unless the init_weight is modified in MMCV. But it's not practical at present. I think you can use another log here to indicate the key encoder is initialized by the query encoder, as a workaround. Thanks!

Hi! I see. So would that be done by using mmselfsup.utils.get_root_logger to get a logger and then log with that, or just a simple print statement?

Better use mmselfsup.utils.get_root_logger

Copy link
Collaborator

@tonysy tonysy left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. LGTM

@fangyixiao18 fangyixiao18 merged commit eff1142 into open-mmlab:dev Aug 19, 2022
@lorinczszabolcs lorinczszabolcs deleted the 404-densecl-weight-init branch August 19, 2022 06:23
fangyixiao18 pushed a commit that referenced this pull request Sep 1, 2022
* DenseCL init weights copy query encoder weights to key encoder.

* Logger prints that key encoder is initialized with query encoder.
fangyixiao18 pushed a commit that referenced this pull request Sep 30, 2022
* DenseCL init weights copy query encoder weights to key encoder.

* Logger prints that key encoder is initialized with query encoder.
fangyixiao18 pushed a commit to fangyixiao18/mmselfsup that referenced this pull request Oct 1, 2022
…-mmlab#411)

* DenseCL init weights copy query encoder weights to key encoder.

* Logger prints that key encoder is initialized with query encoder.
fangyixiao18 pushed a commit that referenced this pull request Oct 1, 2022
* DenseCL init weights copy query encoder weights to key encoder.

* Logger prints that key encoder is initialized with query encoder.
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