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

Adds Backbones to Faster RCNN #382

Closed
wants to merge 4,037 commits into from
Closed

Adds Backbones to Faster RCNN #382

wants to merge 4,037 commits into from

Conversation

oke-aditya
Copy link
Contributor

@oke-aditya oke-aditya commented Nov 19, 2020

What does this PR do?

Closes #340

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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • 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

cc @ananyahjha93

Anyone in the community is free to review the PR once the tests have passed.

Did you have fun?

Make sure you had fun coding 🙃 Obviosuly 😆

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #382 (2a2bfdb) into master (13863cc) will decrease coverage by 0.19%.
The diff coverage is 51.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
- Coverage   80.79%   80.60%   -0.20%     
==========================================
  Files         100      105       +5     
  Lines        5728     5800      +72     
==========================================
+ Hits         4628     4675      +47     
- Misses       1100     1125      +25     
Flag Coverage Δ
cpu 25.33% <30.76%> (+0.10%) ⬆️
pytest 25.33% <30.76%> (+0.10%) ⬆️
unittests 79.91% <47.25%> (-0.26%) ⬇️

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

Impacted Files Coverage Δ
...dels/detection/components/torchvision_backbones.py 20.37% <20.37%> (ø)
pl_bolts/models/detection/faster_rcnn/backbones.py 90.90% <90.90%> (ø)
pl_bolts/datasets/dummy_dataset.py 100.00% <100.00%> (ø)
pl_bolts/models/detection/__init__.py 100.00% <100.00%> (ø)
pl_bolts/models/detection/components/__init__.py 100.00% <100.00%> (ø)
...s/models/detection/components/_supported_models.py 100.00% <100.00%> (ø)
pl_bolts/models/detection/faster_rcnn/__init__.py 100.00% <100.00%> (ø)
...models/detection/faster_rcnn/faster_rcnn_module.py 81.66% <100.00%> (ø)
pl_bolts/callbacks/variational.py 97.50% <0.00%> (-0.18%) ⬇️
pl_bolts/__init__.py 100.00% <0.00%> (ø)
... and 12 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 13863cc...35fab7b. Read the comment docs.

@oke-aditya
Copy link
Contributor Author

create_torchvision_backbone is a hijacked way of making the backbone. It basically removes the last linear layer and keeps all the other layer to nn.Sequential module.

There is lot of scope to improve it to make transfer learning better.

  1. Module unfreezing, We can freeze some layers to allow certain layer of numbers to be trained.
  2. Extend to other backbone.

We can iterate on that further.

These are really components and can be used by RetinaNet as well or for Transfer learning with CNNs too. #286.

I slightly rearranged the FRCNN codebase, and it's classname since it collides with torchvision.

@Borda Borda added this to In progress in Object detection Nov 20, 2020
@Borda Borda added enhancement New feature or request model labels Nov 22, 2020
Object detection automation moved this from In progress to in Review Nov 22, 2020
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls, simplify the model selection...

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@oke-aditya Thank you for your contribution as always :] I left some comments. Mind having a look?

.gitignore Show resolved Hide resolved
pl_bolts/models/detection/faster_rcnn/backbones.py Outdated Show resolved Hide resolved
pl_bolts/models/detection/faster_rcnn/backbones.py Outdated Show resolved Hide resolved
teddykoker and others added 17 commits November 24, 2020 02:07
…n self.log (#4327)

* update logging.rst

* logger of choice

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>

* add metrics reference

* trigger ci

* Revert "trigger ci"

This reverts commit 97bf461cf9c00d182b0cc841c6b966a0ca9e85a4.

Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Roger Shieh <sh.rog@protonmail.ch>
Co-authored-by: Ananya Harsh Jha <ananya@pytorchlightning.ai>
* update test

* docstring

Co-authored-by: Ananya Harsh Jha <ananya@pytorchlightning.ai>
* precision

* precision

* recall

* f beta

* confusion matrix

* mse

* mae

* msle

* expalained variance

* psnr

* ssim

* text fp fn tp

* accuracy

* wiki -> sklearn for confusion metrix link

* confusion matrix logging note

Co-authored-by: Roger Shieh <sh.rog@protonmail.ch>
@oke-aditya
Copy link
Contributor Author

oke-aditya commented Nov 25, 2020

I shifted these above to a new file to avoid clutter.
Somehow I'm really not sure how CI works here and why the Base testing is failing, please help me in fixing that.

One more proposal is
Load by defualt with pretrained=None, which will allow us to use any other possible weights.
These weights support can then be made available using pretrained: str = "imagenet" and so on paramters.

What we do is additionally maintain a file with _pretrained_weights`

This helps us to load pretrained weights given the string parameter.

So people can possibly do the following with create_torchvision_backbone

create_torchvision_backbone("resnet50", pretrained="imagnet")
create_torchvision_backbone("resnet50", pretrained="instagram") # Example

Also FRCNN gets these superpowers with simple string parameter.

model = FRCNN(num_classes=91, pretrained="imagenet", backbone="resnet50")

This makes a bit more powerful and allows people to easily publish their own weights using a simple url to the file.
This serves as a simple hub.

This collides with #200 thoughts and I leave this optional here as this PR is meant for object detection.

We can discuss this in a new issue too.

cc @Borda @akihironitta

rohitgr7 and others added 8 commits December 20, 2020 08:50
* Remove Sourcerer

* trigger

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
* skip test

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
Co-authored-by: Roger Shieh <sh.rog@protonmail.ch>
* remove unused rpc import

* isort

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
* draft

* fix

* drop folder

Co-authored-by: chaton <thomas@grid.ai>
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@oke-aditya Thank you for your work. I'm sorry I'm replying late. I left some comments, so would you mind having a look at them?


Also, I think we need some tests for newly added public functions:

create_torchvision_backbone
create_fasterrcnn_backbone

Comment on lines +78 to +82
out_channels = 512
model_selected = TORCHVISION_MODEL_ZOO[model_name]
net = model_selected(pretrained=pretrained)
ft_backbone = _create_backbone_features(net, out_channels)
return ft_backbone, out_channels
Copy link
Contributor

Choose a reason for hiding this comment

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

@oke-aditya How about moving

model_selected = TORCHVISION_MODEL_ZOO[model_name]
net = model_selected(pretrained=pretrained)

out of if/elif/else? The current implementation looks somewhat repetitive...

warn_missing_pkg('torchvision') # pragma: no-cover


def _create_backbone_generic(model: nn.Module, out_channels: int):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add return types for all functions?

Comment on lines +78 to +82
out_channels = 512
model_selected = TORCHVISION_MODEL_ZOO[model_name]
net = model_selected(pretrained=pretrained)
ft_backbone = _create_backbone_features(net, out_channels)
return ft_backbone, out_channels
Copy link
Contributor

Choose a reason for hiding this comment

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

@oke-aditya Also, I think that, instead of returning in each if/elif/else, having only one return statement in this function will look better.


def create_torchvision_backbone(model_name: str, pretrained: bool = True):
"""
Creates CNN backbone from Torchvision.
Copy link
Contributor

Choose a reason for hiding this comment

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

A tiny fix for consistency :]

Suggested change
Creates CNN backbone from Torchvision.
Creates CNN backbone from torchvision.


Args:
model_name: Name of the model. E.g. resnet18
pretrained: Pretrained weights dataset "imagenet", etc
Copy link
Contributor

Choose a reason for hiding this comment

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

Load by defualt with pretrained=None, which will allow us to use any other possible weights.
These weights support can then be made available using pretrained: str = "imagenet" and so on paramters.

For now, let's match the docstring of arg pretrained with pretrained (bool) in torchvision. (docs) Should be further discussed in #200.

Comment on lines +12 to +13
def create_fasterrcnn_backbone(backbone: str, fpn: bool = True, pretrained: str = None,
trainable_backbone_layers: int = 3, **kwargs) -> nn.Module:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def create_fasterrcnn_backbone(backbone: str, fpn: bool = True, pretrained: str = None,
trainable_backbone_layers: int = 3, **kwargs) -> nn.Module:
def create_fasterrcnn_backbone(backbone: str, fpn: bool = True, pretrained: str = None,
trainable_backbone_layers: int = 3, **kwargs: Any) -> nn.Module:

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the name create_fasterrcnn_backbone sounds a bit limiting. The function can be used for other detection algorithms, right?

@akihironitta
Copy link
Contributor

akihironitta commented Dec 22, 2020

@oke-aditya Would you resolve the conflicts, too? (Just updated the title of this PR since we might need some more work...)

@akihironitta akihironitta changed the title [Ready to Merge] Adds Backbones to Faster RCNN Adds Backbones to Faster RCNN Dec 22, 2020
@oke-aditya
Copy link
Contributor Author

oke-aditya commented Dec 22, 2020

Yeah sure,

alanhdu and others added 14 commits December 23, 2020 12:35
* update

* clean test

* still in progress

* udpdate test

* update

* update

* resolve flake

* add test for zero_grad

* update

* works without accumulated_grad

* update

* update

* resolve amp

* revert back to True

* update

* clean tests

* cleaned out

* typo

* update test

* git repare bug

* remove print

* udpate

* Fix formatting/optimizer imports

* Refactor the test for cleanliness

* Add vanilla model to the test, better var names

* Fixed var names, let's clean up these mock tests

* repare test

* update test

* resolve flake8

* add manual_optimization

* update tests

* resolve flake8

* add random accumulate_grad_batches

* improve test

* Update tests/trainer/optimization/test_parity_automatic_optimization.py

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

* Update tests/trainer/optimization/test_parity_automatic_optimization.py

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

* update

* clean tests

* correct bug

* Apply suggestions from code review

* format

* adress comments

* update on comments

Co-authored-by: SeanNaren <sean@grid.ai>
Co-authored-by: Ubuntu <ubuntu@ip-172-31-62-109.ec2.internal>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <jirka.borovec@seznam.cz>
* update chlog for future 1.1.3rc

* prune

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
* [bugfix] Group defaults to WORLD if None

* fix no_grad

* Update pytorch_lightning/utilities/distributed.py

* Update pytorch_lightning/utilities/distributed.py

Co-authored-by: Gregor Koporec <gregork@unicorn.gorenje.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Sean Naren <sean.narenthiran@gmail.com>
* refactor

* memory

* show

* clean

* clean

* try

* device

* reset

* fix

* fix

* mean

* hook

* format

* add todo

Co-authored-by: chaton <thomas@grid.ai>

Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
* skip some description from pypi

* flake8
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
* Add TPU example

* add badge

* add badge

* add badge

* bullets

* name

* trigger

* add dataset name

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: chaton <thomas@grid.ai>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
@oke-aditya
Copy link
Contributor Author

oke-aditya commented Dec 24, 2020

😓 Looks like I smashed the git history. (Too many conflicts and something went wrong).
Let me open a new PR

I will pick up the changes from the previous commit and add stuff in new PR.

All I intended to do was merge master into this branch and accept all incoming changes if any conflict.
Looks like there were too many and somehow this was complete mess. (Super mess)

@oke-aditya oke-aditya closed this Dec 24, 2020
Object detection automation moved this from in Review to Done Dec 24, 2020
@oke-aditya oke-aditya mentioned this pull request Dec 24, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request model
Projects
Development

Successfully merging this pull request may close these issues.

Enhancing Object Detection