Skip to content

Return loss information for detection models#1605

Open
Delaunay wants to merge 5 commits intopytorch:mainfrom
Delaunay:master
Open

Return loss information for detection models#1605
Delaunay wants to merge 5 commits intopytorch:mainfrom
Delaunay:master

Conversation

@Delaunay
Copy link

Fix for #1574

Why:
Current code assume that loss is only useful in training mode.
This is not true, users might want validation and test loss.

How:

  1. We replace current logic of checking for training mode by checking if targets are provided.
    If targets are provided then we assume users are interested in loss.

  2. We always return a loss dictionary even if it is empty.
    This makes the API consistent and users do not have to change code between eval and training mode

@Delaunay Delaunay changed the title compute loss if targets is present instead of checking for self.training Return loss information for detection model Nov 22, 2019
@Delaunay Delaunay changed the title Return loss information for detection model Return loss information for detection models Nov 22, 2019
@codecov-io
Copy link

codecov-io commented Nov 23, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5b1716a). Click here to learn what that means.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1605   +/-   ##
=========================================
  Coverage          ?   65.67%           
=========================================
  Files             ?       90           
  Lines             ?     7083           
  Branches          ?     1074           
=========================================
  Hits              ?     4652           
  Misses            ?     2125           
  Partials          ?      306
Impacted Files Coverage Δ
torchvision/models/detection/roi_heads.py 51.13% <0%> (ø)
torchvision/models/detection/rpn.py 82.19% <0%> (ø)
torchvision/models/detection/generalized_rcnn.py 84.61% <100%> (ø)

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 5b1716a...f1fe576. Read the comment docs.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Hi,

I like the idea of this change.
It will even make it easier for making the model be torchscript-compatible, because torchscript can't handle dynamic return values.

This is unfortunately a BC-breaking change, so we can't merge this as is for now. We will be cutting a new release of torchvision in the beginning of January, and that version will have a deprecation warning saying that we will be changing the return values to be always loss, detections.
One more thing is that we now have a very unintuitive behavior: if we pass the targets, we don't compute the postprocessed predictions, but if we don't, then they are postprocessed. I think we should iterate a bit on that in January, maybe by adding a new argument to the model to postprocess the outputs.

In the meantime, the training scripts in references/detection/ are now broken because of this change, and should be fixed before merge.

@Delaunay
Copy link
Author

Delaunay commented Dec 2, 2019

Merged the master branch to get the torch.jit changes as well.
Failing tests are the same as the master branch.

Todos

  • updated references/detection/
  • detect when to postprocess predictions (if targets are passed they are not)
    • ? adding a new argument to the model to postprocess the outputs.

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.

4 participants