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

Add RetinaNet Object detection with Backbones #529

Merged
merged 150 commits into from Dec 20, 2021

Conversation

oke-aditya
Copy link
Contributor

@oke-aditya oke-aditya commented Jan 19, 2021

What does this PR do?

Fixes #391

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?

Did you have fun?

I think yes 😛

@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #529 (857a19f) into master (7369ad9) will increase coverage by 0%.
The diff coverage is 78%.

❗ Current head 857a19f differs from pull request most recent head 9817ced. Consider uploading reports for the commit 9817ced to get more accurate results

@@          Coverage Diff           @@
##           master   #529    +/-   ##
======================================
  Coverage      73%    73%            
======================================
  Files         125    131     +6     
  Lines        7884   8552   +668     
======================================
+ Hits         5737   6259   +522     
- Misses       2147   2293   +146     

@oke-aditya
Copy link
Contributor Author

oke-aditya commented Jan 20, 2021

This PR also bumps minimum torchvision to 0.8.1 and PyTorch to 1.7.
This is must to include retinanet as it comes from torchvision 0.8.1 only
I will ensure that all other tests pass with this upgrade.
We can possibly enable testing for Python 3.9 too with this (do in later PR to avoid unrelated failure)

Also this version bump enables us to use CUDA 11 compatible GPUs Ampere Architecture (A100, RTX30XX series).
It might fix distributed problems over Windwos as Compatibility is improved (with Gloo backend).

cc @Borda @akihironitta @ananyahjha93

@oke-aditya oke-aditya changed the title [WIP] Add RetinaNet Object detection with Backbones Add RetinaNet Object detection with Backbones Jan 20, 2021
Copy link
Contributor Author

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

I left some comments to ease further discussions :)

pl_bolts/metrics/object_detection.py Outdated Show resolved Hide resolved
pl_bolts/metrics/object_detection.py Outdated Show resolved Hide resolved
tests/models/test_detection.py Outdated Show resolved Hide resolved
@oke-aditya
Copy link
Contributor Author

The RetinaNet test is very slow, the reason being not using Batched NMS implementation. This is fixed in torchvision master and we can expect faster RetinaNet model + tests in next torchvision release

oke-aditya and others added 3 commits January 23, 2021 20:03
Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
@oke-aditya
Copy link
Contributor Author

oke-aditya commented Jan 25, 2021

@Borda @akihironitta can you have look 😅

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.

It'd be nice to hear your thoughts!

pl_bolts/models/detection/retinanet/retainanet_module.py Outdated Show resolved Hide resolved
tests/models/test_detection.py Outdated Show resolved Hide resolved
@akihironitta akihironitta mentioned this pull request Dec 18, 2021
5 tasks
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.

I believe this PR is ready for getting merged.

Just for the note, here's the summary of my last tens of commits:

  • Updated CHANGELOG
  • Added its doc
  • Reverted the change in Faster R-CNN as it's irrelevant to this PR
  • Made RetinaNet use torchvision.ops.box_iou instead of our in-house pl_bolts.metrics.iou
  • Resolved all the failing tests related to this PR

cc @Borda

@Borda
Copy link
Member

Borda commented Dec 18, 2021

Just for the note, the currently failing tests are irrelevant to this PR. The failures are due to PyTorchLightning/pytorch-lightning#9535 where the error occurs only with Python 3.6 and pytorch_lightning<1.4.8. The fix (PyTorchLightning/pytorch-lightning#9554) is included in >=1.4.8.

so if we set py3.7 as the min version then we do not need to upgrade PL?

UPDATE: also if we replace the argparse with LightningCLI we are fine with the PL 1.4.0?

This reverts commit 5fccad6, reversing
changes made to c30e68e.
@akihironitta
Copy link
Contributor

akihironitta commented Dec 19, 2021

TODO:

  • Set the min PL version to 1.4.0
  • Remove argparse and use LightningCLI

@mergify mergify bot removed the ready label Dec 19, 2021
@mergify mergify bot added the ready label Dec 19, 2021
@oke-aditya
Copy link
Contributor Author

Looks great. So kind of you @akihironitta Aki : 💖

@mergify mergify bot removed the has conflicts label Dec 20, 2021
@Borda Borda merged commit 51edf4a into Lightning-Universe:master Dec 20, 2021
@oke-aditya
Copy link
Contributor Author

Thanks a lot to @akihironitta and @Borda for your patience. I'm Extremely sorry for this being delayed.

@akihironitta
Copy link
Contributor

@oke-aditya Hey Aditya, not at all! Thank you for your work and patience for almost a year 🎉

@oke-aditya oke-aditya deleted the add_retina branch December 20, 2021 13:35
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.

Add RetinaNet Object Detection
8 participants