lightning-bolts
lightning-bolts copied to clipboard
Revision of SimCLR transforms
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
- [x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
- [x] Did you read the contributor guideline, Pull Request section?
- [x] 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?
- [x] 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 🙃
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.
* _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 :joy: 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 :rocket:
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 :zap: