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

Revision of SimCLR transforms #857

Merged
merged 18 commits into from Sep 16, 2022

Conversation

ArnolFokam
Copy link
Contributor

@ArnolFokam ArnolFokam commented Aug 6, 2022

What does this PR do?

Fixes part of #839

  • pl_bolts.models.self_supervised.simclr.transforms.GaussianBlur (removed in favor of torchvision's implementation)
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLREvalDataTransform (add unit test)
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLRFinetuneTransform (add unit test and doc string)
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLRTrainDataTransform (add unit test)

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@github-actions github-actions bot added the model label Aug 6, 2022
@ArnolFokam
Copy link
Contributor Author

Concerning my PR, I'm not yet sure if it's up to the repo's standards. Therefore, I have the following questions before I can complete my work.

  • Did you make sure to update the documentation with your changes? Does this only concerns the doc string comments?
  • Did you verify new and existing tests pass locally with your changes? Running make test takes ages and is eventually being killed by my os (Ubuntu 20.04.2 LTS WSL on Windows 11). Only ran a subset of tests.

Thanks.

@ArnolFokam ArnolFokam marked this pull request as ready for review August 6, 2022 20:38
@ArnolFokam ArnolFokam changed the title [WIP] Revision of SimCLR transforms Revision of SimCLR transforms Aug 6, 2022
@otaj otaj mentioned this pull request Aug 12, 2022
Comment on lines 123 to 139
"""Transforms for SimCLR during the fine-tuning stage.

Transform::

Resize(input_height + 10, interpolation=3)
transforms.CenterCrop(input_height),
transforms.ToTensor()

Example::

from pl_bolts.models.self_supervised.simclr.transforms import SimCLREvalDataTransform

transform = SimCLREvalDataTransform(input_height=32)
x = sample()
(_, _, xk) = transform(x)
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I'm not too versed in SimCLR, but either this docstring is wrong or the __call__ is wrong since it definitely does not return 3-tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting this ❤️

Copy link
Contributor

@otaj otaj left a comment

Choose a reason for hiding this comment

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

Hi, I'm sorry it took me so long. Overall it looks good, I'd just take a bit harder look at SimCLRFinetuneTransform. ⚡

@@ -129,8 +119,24 @@ def __init__(
)


@under_review()
class SimCLRFinetuneTransform:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this somehow be also subclass of SimCLRTrainTransform? Seems like quite a bit of code is duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do.

@otaj
Copy link
Contributor

otaj commented Aug 15, 2022

* _Did you make sure to update the documentation with your changes?_ Does this only concerns the doc string comments?

That usually concerns whether documentation is up to date with respect to your changes. Quite a bit of documentation is generated from docstrings, so you should be okay with that. However, you can take a look yourself locally by running make docs and taking a look at docs/build

* _Did you verify new and  **existing tests** pass locally with your changes?_ Running `make test` takes ages and is eventually being killed by my os (Ubuntu 20.04.2 LTS WSL on Windows 11). Only ran a subset of tests.

This depends on the specs of your machine I'm afraid. On my local machine (32GB, i7-11850H, T1200 4GB) it takes about 20 min to run everything and 2 of the tests fail because my GPU runs out of memory 😂 Try to run as much as possible and if you're not unable to test something locally, it's fine to use CI to warn you about any failing tests

Which reminds me, you had one test failure on the PR on minimal installation (which tests whether everything works if we explicitly install the lowest versions indicated in requirements.txt). Those are two warnings being raising from torchvision. I'm perfectly fine with either raising torchvision version or silencing these warnings, decision is yours 🚀

@otaj otaj self-assigned this Sep 15, 2022
@otaj
Copy link
Contributor

otaj commented Sep 15, 2022

Hi, @ArnolFokam! Sorry for revoking "the decision is yours", but it seems if we bump the torchvision, we also need to bump torch, since those versions are very much interlinked. And we don't want to do that, mostly because of PL, we want to be able to support as many versions as PL supports. I took the liberty of reverting that and silencing the warning. If the CI passes, I will approve it and merge it ⚡

@mergify mergify bot added the ready label Sep 15, 2022
@otaj otaj enabled auto-merge (squash) September 15, 2022 16:34
@otaj otaj merged commit 2a1f134 into Lightning-Universe:master Sep 16, 2022
@ArnolFokam ArnolFokam deleted the feature/simclr_transforms branch September 16, 2022 14:21
Jungwon-Lee pushed a commit to Jungwon-Lee/lightning-bolts that referenced this pull request Sep 18, 2022
Co-authored-by: otaj <ota@lightning.ai>
Co-authored-by: arnol <fokammanuel1@students.wits.ac.za>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants