fix resnet_fpn_backbone docstring, allow backbone to be pluggable #2499
Conversation
| @@ -5,7 +5,6 @@ | |||
| from torchvision.models.detection.image_list import ImageList | |||
| from torchvision.models.detection.transform import GeneralizedRCNNTransform | |||
| from torchvision.models.detection.rpn import AnchorGenerator, RPNHead, RegionProposalNetwork | |||
| from torchvision.models.detection.backbone_utils import resnet_fpn_backbone | |||
jimexist
force-pushed
the
add-callable-backbone-util
branch
from
fdb5b39
to
06610ef
Compare
|
cc @muaz-urwa |
Codecov Report
@@ Coverage Diff @@
## master #2499 +/- ##
==========================================
+ Coverage 70.60% 70.62% +0.01%
==========================================
Files 94 94
Lines 7995 7999 +4
Branches 1272 1274 +2
==========================================
+ Hits 5645 5649 +4
Misses 1947 1947
Partials 403 403
Continue to review full report at Codecov.
|
jimexist
force-pushed
the
add-callable-backbone-util
branch
2 times, most recently
from
f42bb0a
to
e19785e
Compare
jimexist
force-pushed
the
add-callable-backbone-util
branch
from
e19785e
to
aad9b50
Compare
There was a problem hiding this comment.
Thanks for the PR!
The resnet_fpn_backbone has many built-in assumptions about the model structure, and as such is not generic enough to accept a callable in my opinion. The same thing happens btw with IntermediateLayerGetter, and that's the reason why it's under a private namespace.
As such, I think it would be preferable to not move forward with this PR, as I think it can lead to more issues and confusion for the users, and thus I'm closing the PR.
Please let me know if you think otherwise.
| if isinstance(backbone, str): | ||
| backbone = resnet.__dict__[backbone](pretrained=pretrained, norm_layer=norm_layer) | ||
| elif callable(backbone): | ||
| backbone = backbone(pretrained=pretrained, norm_layer=norm_layer) |
There was a problem hiding this comment.
This assumes that the callable has a particular interface and its modules follow the same convention of resnets, which is not always the case and will fail for most of the models in torchvision. As such, I think it might be better to avoid trying to be generic in here, given that this function is not meant to be generic.
this PR addresses two issues:
resnet_fpn_backboneresnet_fpn_backboneto be used with compatible backbones other than the list provided inresnetmodule, e.g. resnext and resnestthe misplaced docstring was added in #2160.