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

Added nn.Module support for FasterRCNN backbone #661

Merged
merged 19 commits into from Nov 15, 2021

Conversation

abhayraw1
Copy link
Contributor

@abhayraw1 abhayraw1 commented Jun 15, 2021

What does this PR do?

Fixes #660

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?

Sure! 🙃

@github-actions github-actions bot added the model label Jun 15, 2021
@abhayraw1 abhayraw1 marked this pull request as ready for review June 16, 2021 10:25
@abhayraw1
Copy link
Contributor Author

@Borda, I made the changes. Let me know what you think!

PS: Tests had failed due to the issue below, which I see has been fixed now...

This change is causing tests to fail in master. Please check!

3bf65da#diff-81b21ad681f00e8e80279aaf4d5e2c3a304b5a587b79850cff8d4472147f7f0aL92

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #661 (164d368) into master (4046278) will decrease coverage by 48.44%.
The diff coverage is 14.28%.

❗ Current head 164d368 differs from pull request most recent head 68cc487. Consider uploading reports for the commit 68cc487 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master     #661       +/-   ##
===========================================
- Coverage   72.82%   24.37%   -48.45%     
===========================================
  Files         121      121               
  Lines        7550     7520       -30     
===========================================
- Hits         5498     1833     -3665     
- Misses       2052     5687     +3635     
Flag Coverage Δ
cpu 24.37% <14.28%> (-48.45%) ⬇️
pytest 24.37% <14.28%> (-48.45%) ⬇️

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

Impacted Files Coverage Δ
...models/detection/faster_rcnn/faster_rcnn_module.py 23.45% <14.28%> (-45.38%) ⬇️
pl_bolts/models/rl/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
pl_bolts/models/rl/common/agents.py 0.00% <0.00%> (-100.00%) ⬇️
pl_bolts/models/rl/dueling_dqn_model.py 0.00% <0.00%> (-100.00%) ⬇️
pl_bolts/models/rl/advantage_actor_critic_model.py 0.00% <0.00%> (-97.71%) ⬇️
pl_bolts/models/rl/double_dqn_model.py 0.00% <0.00%> (-95.84%) ⬇️
...l_bolts/models/rl/vanilla_policy_gradient_model.py 0.00% <0.00%> (-93.45%) ⬇️
pl_bolts/models/rl/common/networks.py 0.00% <0.00%> (-92.31%) ⬇️
pl_bolts/models/rl/reinforce_model.py 0.00% <0.00%> (-89.40%) ⬇️
pl_bolts/models/rl/per_dqn_model.py 0.00% <0.00%> (-86.89%) ⬇️
... and 68 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 fa7d1df...68cc487. Read the comment docs.

@Borda Borda added enhancement New feature or request waiting on author labels Jun 24, 2021
@Borda Borda changed the title Added torch.nn.Module (and pl.LightningModule) support for FasterRCNN backbone Added nn.Module (and pl.LightningModule) support for FasterRCNN backbone Aug 13, 2021
@Borda Borda changed the title Added nn.Module (and pl.LightningModule) support for FasterRCNN backbone Added nn.Module support for FasterRCNN backbone Aug 25, 2021
@Borda
Copy link
Member

Borda commented Aug 25, 2021

@abhayraw1 could you pls check the failing test (which are passing on master)? 🐰

@abhayraw1
Copy link
Contributor Author

Sure @Borda

@SeanNaren SeanNaren enabled auto-merge (squash) September 8, 2021 11:15
@SeanNaren
Copy link
Contributor

Hey @abhayraw1 seems there is one failing test left, do you mind checking it out! thanks for your work so far!!

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.

@abhayraw1 LGTM!

@Borda
Copy link
Member

Borda commented Sep 10, 2021

Not sure what is wrong here:

                boxes = target["boxes"]
                degenerate_boxes = boxes[:, 2:] <= boxes[:, :2]
                if degenerate_boxes.any():
                    # print the first degenerate box
                    bb_idx = torch.where(degenerate_boxes.any(dim=1))[0][0]
                    degen_bb: List[float] = boxes[bb_idx].tolist()
>                   raise ValueError("All bounding boxes should have positive height and width."
                                     " Found invalid box {} for target at index {}."
                                     .format(degen_bb, target_idx))
E                   ValueError: All bounding boxes should have positive height and width. Found invalid box [137.5, 556.25, 743.75, 556.25] for target at index 0.

as I see the [137.5, 556.25] <= [743.75, 556.25] shall be true, but it fails...

Copy link
Contributor

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

LGTM 😃

@mergify mergify bot added the ready label Nov 15, 2021
@SeanNaren SeanNaren merged commit d58a9b9 into Lightning-Universe:master Nov 15, 2021
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 ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability add custom/modified backbone to FasterRCNN
5 participants