Add backbones

[oke-aditya]

• I have added the code to support most torchvision bacbones (except alexnet and Densenet)

I have unit tested the mantisshrimp/backbones/torchvision_backbones.py file.

How do I test the entire fasterRCNN code I am unsure. Please let me know so that I can test and create PRs next time.

[codecov-commenter]

Codecov Report

Merging #64 into master will increase coverage by 2.49%.
The diff coverage is 98.78%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   53.51%   56.00%   +2.49%     
==========================================
  Files          53       55       +2     
  Lines        1637     1714      +77     
==========================================
+ Hits          876      960      +84     
+ Misses        761      754       -7     

Flag	Coverage Δ
#unittests	56.00% <98.78%> (+2.49%)	⬆️

Impacted Files	Coverage Δ
mantisshrimp/backbones/torchvision_backbones.py	98.46% <98.46%> (ø)
mantisshrimp/backbones/__init__.py	100.00% <100.00%> (ø)
...tisshrimp/models/mantis_rcnn/mantis_faster_rcnn.py	96.96% <100.00%> (+27.40%)	⬆️
mantisshrimp/test_utils/samples.py	90.90% <0.00%> (+9.09%)	⬆️

---

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 864ffe3...97648a2. Read the comment docs.

[lgvaz]

Thanks for the PR!

There are some suggestions I would like to give before merging

Currently, how can a user specify a custom backbone that is not defined in create_torchvision_backbone? The way is implemented right now he would need to change the source code

What do you think of having something like

backbone = get_torchvision_backbone('mobile_net')
model = MantisFasterRCNN(backbone=backbone, ...)

Also, keep in mind that we want to support pytorch hub in the future, how can we write something that we can easily integrate with that?

[lgvaz]

Another tip, we can mention the issue here so it gets automatically closed when this merges =)

Closes #61

[oke-aditya]

Thanks for the PR!

There are some suggestions I would like to give before merging

Currently, how can a user specify a custom backbone that is not defined in create_torchvision_backbone? The way is implemented right now he would need to change the source code

What do you think of having something like

backbone = get_torchvision_backbone('mobile_net')
model = MantisFasterRCNN(backbone=backbone, ...)

Also, keep in mind that we want to support pytorch hub in the future, how can we write something that we can easily integrate with that?

For Pytorch hub, we could simply take CNN features from there. Maybe create a file in backbone folder as get_hub_backbones and make use of them here.

For user specified backbones, I'm unsure, user would have to write their CNN architecture and get features from their CNN architecture. This needs to be passed to our our code.

[oke-aditya]

I have made changes, to solve other stuff but we would need to think to accommodate those two as well.

[oke-aditya]

Also, I just checked the research paper of FasterRCNN. Originally the backbones were trained with ImageNet and the other layers were fine-tuned on COCO.

So the torchvision layers with pretrained ImageNet weights should be fine.

[oke-aditya]

As suggested in the paper Anchors are not optimized in FasterRCNN it should be taken care as per the dataset needs. I guess users are aware of this.

[lgvaz]

For tests, we use the pytest framework, it's really simple, you just need to write test functions, take a look at tests/utils/test_imageio.py for a very simple example.

So basically, just create a file in the tests directory following the same folder structure of the implemented files, and start test very simple stuff. For the backbones, I suggested that you test the model output shapes, so create a dummy input (it can be a tensor full of zeros), feed to the network, and check if the shape is what you expected.

For FasterRCNN you do something similar, instantiate it and see if the output is what you expected.

[lgvaz]

Do not create a new PR with the tests, instead keep adding to this one. When everything is done I'll merge it =)

[oke-aditya]

Backbones tests done. I will add the Faster RCNN tests.

[lgvaz]

This is kinda of what I had in mind:

class MantisFasterRCNN(MantisRCNN):
    @delegates(FasterRCNN.__init__)
    def __init__(self, n_class, backbone=None, metrics=None, **kwargs):
        super().__init__(metrics=metrics)
        self.n_class = n_class

        if backbone is None:
            # Creates the default fasterrcnn as given in pytorch. Trained on COCO dataset
            self.m = fasterrcnn_resnet50_fpn(pretrained=True, **kwargs)
            in_features = self.m.roi_heads.box_predictor.cls_score.in_features
            self.m.roi_heads.box_predictor = FastRCNNPredictor(in_features, n_class)
        else:
            self.m = FasterRCNN(backbone=backbone, num_classes=n_class, **kwargs)

[lgvaz]

Actually, looking at torchvisions source code, I see they already have a custom function for getting backbones resnet_fpn_backbone, it also add FPN on top which is nice.

Any disadvantages of using this instead of create_torchvision_backbone?

You can take a look here for a guide on how to correctly set it up =)

[oke-aditya]

It has support for all ResNet models. Suppose we want to extend for other models such as VGG 16 (originally in paper) and mobilenet. Or in future from hub. We would need a generalized function. Hence I thought of this.

This is provided in the same util which you gave. I guess we need to add create_fpn util which will create fpn for any backbone.

As of now. Let us choice for ResNets with FPNs or without and and ResNet + other nets or own CNNs without fpn.

Maybe below blocks will help us extend this further.

[oke-aditya]

https://pytorch.org/docs/stable/_modules/torchvision/models/detection/faster_rcnn.html#fasterrcnn_resnet50_fpn

As suggested we could pass the backbones as well, but it would create without fpn. I guess we can have an extra user argument to specify if they are using resnet, they need fpn or not.

class BackboneWithFPN(nn.Module):
    """
    Adds a FPN on top of a model.
    Internally, it uses torchvision.models._utils.IntermediateLayerGetter to
    extract a submodel that returns the feature maps specified in return_layers.
    The same limitations of IntermediatLayerGetter apply here.
    Arguments:
        backbone (nn.Module)
        return_layers (Dict[name, new_name]): a dict containing the names
            of the modules for which the activations will be returned as
            the key of the dict, and the value of the dict is the name
            of the returned activation (which the user can specify).
        in_channels_list (List[int]): number of channels for each feature map
            that is returned, in the order they are present in the OrderedDict
        out_channels (int): number of channels in the FPN.
    Attributes:
        out_channels (int): the number of channels in the FPN
    """
    def __init__(self, backbone, return_layers, in_channels_list, out_channels):
        super(BackboneWithFPN, self).__init__()
        self.body = IntermediateLayerGetter(backbone, return_layers=return_layers)
        self.fpn = FeaturePyramidNetwork(
            in_channels_list=in_channels_list,
            out_channels=out_channels,
            extra_blocks=LastLevelMaxPool(),
        )
        self.out_channels = out_channels

    def forward(self, x):
        x = self.body(x)
        x = self.fpn(x)
        return x

This code taken from the same util might help us to add fpn to any backbone,

[oke-aditya]

I just checked the FPN paper. It was in 2017. Faster RCNN was done in 2014. So thats why torchvision has these 2 features. It wasn't part of orignal paper.

From the FPN paper

This process is independent of the backbone convolutional architecture 

FPN is completely optionally, Faster RCNN can work without it. So let us give users the choice.
They can use both, as per their experimentations.

[oke-aditya]

I have made the tests ultra granular now, non resnets non fpns, resnets not fpns, resnets fpns.
we do not support non resnets fpns as of now.

But this is best testing I can try

[lgvaz]

Alright! Initially I was a bit hesitant of making these following suggestions because what we have here is already very reasonable, but then I though "this should be a learning experience, so it's worth to spend some of my time showing how we can implement this in a even better and clearer way". So these are my thoughts:

Whenever you start seeing multiple indented blocks, it's a bad sign. Always try to stay away from if statements, of course, an if here and there is okay, but when they start to get nested caos starts to emerge (you can find multiple articles online saying why they are a bad).

Good news is that we always can refactor then, and this is how to do it:

Instead of allowing the user to pass multiple types to backbone at the constructor, restrict it to nn.Module, this way, our new __init__ looks like so:

    def __init__(
        self, n_class: int, backbone: nn.Module = None, metrics=None, **kwargs,
    ):
        super().__init__(metrics=metrics)
        self.n_class = n_class
        self.backbone = backbone

        if backbone is None:
            # Creates the default fasterrcnn as given in pytorch. Trained on COCO dataset
            self.m = fasterrcnn_resnet50_fpn(
                pretrained=True, num_classes=n_class, **kwargs,
            )
            in_features = self.m.roi_heads.box_predictor.cls_score.in_features
            self.m.roi_heads.box_predictor = FastRCNNPredictor(in_features, n_class)
        else:
            self.m = FasterRCNN(backbone, num_classes=n_class, **kwargs)

Much simpler right? And note that the amount of parameters we need to specify at __init__ also dropped significantly.

So now the question is, how to allow the user to specify the backbone by passing a str? That's is certainly very handy...

Well, create a new function for that!

    @staticmethod
    def get_backbone_by_name(
        name: str, fpn: bool = True, pretrained: bool = True
    ) -> nn.Module:
        # Giving string as a backbone, which is either supported resnet or backbone
        if fpn:
            # Creates a torchvision resnet model with fpn added
            # Will need to add support for other models with fpn as well
            # Passing pretrained True will initiate backbone which was trained on ImageNet
            if name in MantisFasterRCNN.supported_resnet_fpn_models:
                # It returns BackboneWithFPN model
                backbone = resnet_fpn_backbone(name, pretrained=pretrained)
            else:
                raise ValueError(f"{name} with FPN not supported")
        else:
            # This does not create fpn backbone, it is supported for all models
            if name in MantisFasterRCNN.supported_non_fpn_models:
                backbone = create_torchvision_backbone(name, pretrained=pretrained)
            else:
                raise ValueError(f"{name} not supported")

        return backbone

Again you see the types of the arguments are very clear, this makes is so we have a very intuitive and easy to use API.

So, if the user wants a resnet101 with fpn he just have to do:

backbone = MantisFasterRCNN.get_backbone_by_name("resnet101")
model = MantisFasterRCNN(2, backbone=backbone)

You can even remove the ifs checking if the model name is in the supported_list and let the errors be naturally thrown (because both resnet_fpn_backbone and create_torchvision_backbone will already throw errors if you pass incorrect names). Then the only thing you need to do is specify the supported names in the function docstring (as you done already for the constructor). And that's it, easy for the user, easy for the devs, easy to maintain 😉. Added benefit that if resnet_fpn_backbone starts supporting new backbones we don't have to keep updating our supported_list

[oke-aditya]

• The issue maybe with loading the state dict to pretrained stuff. This causes github cache I guess.
• This time trying without True pretrained

[lgvaz]

Nice! I'll pull the PR locally and take a look at the tests.

I'll then modify what it's needed to fix then and merge, cool?

[oke-aditya]

Absolutely fine 😀

[oke-aditya]

pytorch/vision#993

This is the problem.

Torchvision does not support this maybe.
But its opposite to what is given in the docs

https://github.com/pytorch/vision/blob/c2e8a00885e68ae1200eb6440f540e181d9125de/torchvision/models/detection/backbone_utils.py#L44

[lgvaz]

Does this covers majority of the uses cases?

@pytest.mark.slow
@pytest.mark.parametrize("pretrained", [False, True])
@pytest.mark.parametrize(
    "backbone, fpn",
    [
        ("mobilenet", False),
        ("vgg11", False),
        ("vgg13", False),
        ("vgg16", False),
        ("vgg19", False),
        ("resnet18", False),
        ("resnet34", False),
        ("resnet50", False),
        ("resnet18", True),
        ("resnet34", True),
        ("resnet50", True),
        # these models are too big for github runners
        # "resnet101",
        # "resnet152",
        # "resnext101_32x8d",
    ],
)
def test_faster_rcnn_nonfpn_backbones(image, backbone, fpn, pretrained):
    backbone = MantisFasterRCNN.get_backbone_by_name(
        name=backbone, fpn=fpn, pretrained=pretrained
    )
    model = MantisFasterRCNN(n_class=3, backbone=backbone)
    model.eval()
    pred = model(image)
    assert isinstance(pred, list)
    assert set(["boxes", "labels", "scores"]) == set(pred[0].keys())

[oke-aditya]

Yes it does. I will do the testing for other cases as well.
We need to document this more carefully than relying on pytest.

[lgvaz]

The problems on the tests were being caused when the model was on .eval mode and did not found any boxes in the image.

Fixed it by testing on training mode, with the additional benefit that it also tests build_training_sample now (indirectly, but okay)

[lgvaz]

Please review the proposed changes, if you approve and the tests pass, I'll go ahead and merge this.

Many thanks!!!

[oke-aditya]

Great. All tests are passing now. 👍 Can't wait for this feature to be merged.