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

Faster RCNN Model + Pascal VOC DataModule #157

Merged
merged 10 commits into from
Aug 22, 2020
Merged

Faster RCNN Model + Pascal VOC DataModule #157

merged 10 commits into from
Aug 22, 2020

Conversation

teddykoker
Copy link
Contributor

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 to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

  1. Implements the Pascal VOC Detection dataset as a DataModule
  2. Implements Faster RCNN as a LightningModule

PR review

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 🙃

@pep8speaks
Copy link

pep8speaks commented Aug 14, 2020

Hello @teddykoker! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-22 19:41:49 UTC

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #157 into master will increase coverage by 1.10%.
The diff coverage is 86.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   90.72%   91.82%   +1.10%     
==========================================
  Files          86       86              
  Lines        4052     4049       -3     
==========================================
+ Hits         3676     3718      +42     
+ Misses        376      331      -45     
Flag Coverage Δ
#unittests 91.82% <86.07%> (+1.10%) ⬆️

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

Impacted Files Coverage Δ
pl_bolts/models/detection/faster_rcnn.py 79.62% <79.62%> (ø)
pl_bolts/datamodules/__init__.py 100.00% <100.00%> (ø)
pl_bolts/datamodules/dummy_dataset.py 100.00% <100.00%> (ø)
pl_bolts/models/detection/__init__.py 100.00% <100.00%> (ø)
pl_bolts/callbacks/self_supervised.py 72.72% <0.00%> (ø)
pl_bolts/models/self_supervised/__init__.py 100.00% <0.00%> (ø)
.../models/autoencoders/basic_vae/basic_vae_module.py 92.50% <0.00%> (ø)
..._bolts/models/self_supervised/cpc/cpc_finetuner.py
pl_bolts/models/self_supervised/ssl_finetuner.py
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa1db0...e2b25cf. Read the comment docs.

@Borda Borda added the enhancement New feature or request label Aug 14, 2020
@teddykoker
Copy link
Contributor Author

@Borda didn't add test for cli, but added test training with DummyDetectionDataset, which I just added.

@teddykoker teddykoker requested a review from Borda August 14, 2020 21:47

model = FasterRCNN()

train_dl = DataLoader(DummyDetectionDataset(), collate_fn=_collate_fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a dummy dataset already. why is this one different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of a tensor label it returns a dict with values {"boxes": FloatTensor[N, 4], "labels": Int64Tensor[N]} as per the standard in pytorch for detection models. Should this be a choice in the existing dummy dataset?

@teddykoker
Copy link
Contributor Author

Also VOCDetectionDataModule won't work right now as they modified an image in the dataset that effects the md5sum of the download. I added a fix to torchvision thats been approved: pytorch/vision#2589

@williamFalcon
Copy link
Contributor

@teddykoker mind fixing the failing tests here?

@teddykoker
Copy link
Contributor Author

@williamFalcon it looks like the minimal version tests are failing because I am using a feature of torchvision introduced in 0.7.0 (our requirements state torchvision>=0.5).

Our options are:

  1. Keep torchvision>=0.5 and remove the ability to specify number of trainable backbone layers in the FasterRCNN
  2. Upgrade torchvision >= 0.7 and keep code as is.

Let me know what you think.

@williamFalcon
Copy link
Contributor

for tests we can make torchvision 0.7 but we need to get rid of the deps in the requirements.txt

@teddykoker
Copy link
Contributor Author

test_detection is passing now; it looks like there are issues with BYOL and SimCLR.

@williamFalcon williamFalcon merged commit 620bfd1 into Lightning-Universe:master Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants