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

MoCo momentum update doesn't update head parameters #1059

Open
SebastianGer opened this issue Jul 18, 2023 · 1 comment
Open

MoCo momentum update doesn't update head parameters #1059

SebastianGer opened this issue Jul 18, 2023 · 1 comment
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@SebastianGer
Copy link

🐛 Bug

In the MoCo implementation, we need to add a head to get the MoCo v2 version of the method. However, in the momentum update, only the encoder's parameters are updated, not the head. For example in version 0.5.0, this was not an issue, since the head replaced encoder.fc. That way, when performing the momentum update of all encoder values, the head was also updated. In its current form, the head is an attribute of MoCo, but not of the encoder.

The original MoCo implementation, on which the bolts implementation is based, also replaces encoder.fc: https://github.com/facebookresearch/moco/blob/main/moco/builder.py

The issue could be resolved by having the head replace the fc layer again, or by explicitly momentum-updating the head.

To Reproduce

I have not run any experiments to confirm this.

Code sample

The heads are set here: https://github.com/Lightning-Universe/lightning-bolts/blob/master/src/pl_bolts/models/self_supervised/moco/moco_module.py#L165

But not updated here: https://github.com/Lightning-Universe/lightning-bolts/blob/master/src/pl_bolts/models/self_supervised/moco/moco_module.py#L261

Expected behavior

The momentum update should update all parameters, including those belonging to the head.

Environment

Installed via pip:

lightning==2.0.5
lightning-bolts @ git+https://github.com/PytorchLightning/lightning-bolts.git@4f910f6c9893eaf206eed6b7df545c8a02043a55
lightning-cloud==0.5.37
lightning-utilities==0.9.0
pytorch-lightning==1.9.5

Additional context

A reason for why this problem occurred might be that MoCo uses the torchvision resnet implementation, while e.g. SimSiam uses the pl_bolts implementation. In torchvision, the fc layer is actually used, while in pl_bolts, it seems like the fc layer is unused. When assuming the pl_bolts version, it therefore makes sense to add the head separately, since replacing the fc layer would do nothing.

@SebastianGer SebastianGer added bug Something isn't working help wanted Extra attention is needed labels Jul 18, 2023
@Borda
Copy link
Member

Borda commented Aug 31, 2023

Thank you for this finding @SebastianGer, would you be interested in investigating how to fix it and send a PR? 🐰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants