Closed LukeSutor closed 8 months ago
@guarin When implementing the use_bias functionality for the ProjectionHead class, won't it now be necessary to change all of the types for the lists inside of the other ProjectionHead classes to include the union of the two types? (e.g. https://github.com/lightly-ai/lightly/blob/e84f85896bcaedeb1ff63b9c4bcf7c9d2885bcdf/lightly/models/modules/heads.py#L162).
@guarin When implementing the use_bias functionality for the ProjectionHead class, won't it now be necessary to change all of the types for the lists inside of the other ProjectionHead classes to include the union of the two types? (e.g.
I don't think it is necessary if the ProjectionHead.__init__
method has the following type:
def __init__(
self,
blocks: List[
Union[
Tuple[int, int, Optional[nn.Module], Optional[nn.Module]], # w/o use_bias term
Tuple[int, int, Optional[nn.Module], Optional[nn.Module], bool], # with use_bias term
]
],
This will allow it to accept either the old lists with 4 entries or the new ones with 5 entries.
And could you add the class to this list in the tests: https://github.com/lightly-ai/lightly/blob/e84f85896bcaedeb1ff63b9c4bcf7c9d2885bcdf/tests/models/test_ProjectionHeads.py#L36-L51
Then it is also tested and we know it works as expected.
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
715844b
) 84.40% compared to head (7322153
) 84.43%. Report is 2 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for the update! The code looks good but it seems that there is now a duplicate MMCRProjectionHead
in the file.
Add MMCR Projection Head as proposed in #1481