dmlc / gluon-cv

Gluon CV Toolkit
http://gluon-cv.mxnet.io
Apache License 2.0
5.83k stars 1.22k forks source link

Method get_icnet doesn't respect root parameter for backbone model #1711

Closed lironglu closed 2 years ago

lironglu commented 3 years ago

Looks like method get_icnet in file gluoncv/model_zoo/icnet.py doesn't respect root parameter for backbone model. When calling ICNet constructor to create one ICNet model instance, it doesn't pass value of parameter root to ICNet.

def get_icnet(dataset='citys', backbone='resnet50', pretrained=False,
              root='~/.mxnet/models', pretrained_base=True, ctx=cpu(0), **kwargs):

    model = ICNet(datasets[dataset].NUM_CLASS, backbone=backbone,
                  pretrained_base=pretrained_base, ctx=ctx, **kwargs)

For example, calling method get_icnet with value 'model/icnet_resnet50_mhpv1' for parameter root, ICNet constructor will still download resnet50 model to folder ~/.mxnet/models even if you can't download resnet50 model to folder 'model/icnet_resnet50_mhpv1'

get_icnet(dataset='mhpv1', backbone='resnet50', root='model/icnet_resnet50_mhpv1', **kwargs)

So the correct implementation should be

def get_icnet(dataset='citys', backbone='resnet50', pretrained=False,
              root='~/.mxnet/models', pretrained_base=True, ctx=cpu(0), **kwargs):

    model = ICNet(datasets[dataset].NUM_CLASS, backbone=backbone,
                  pretrained_base=pretrained_base, ctx=ctx, root=root, **kwargs)
lironglu commented 3 years ago

Looks like there is the same issue for many other models. For example, deeplab. In file gluoncv/model_zoo/deeplabv3.py, there is the similar implementation. The backbone model doesn't respect value of root too.

def get_deeplab(dataset='pascal_voc', backbone='resnet50', pretrained=False,
            root='~/.mxnet/models', ctx=cpu(0), **kwargs):
    ....
    model = DeepLabV3(datasets[dataset].NUM_CLASS, backbone=backbone, ctx=ctx, **kwargs)
bryanyzhu commented 2 years ago

We try to make sure all our models are in one place for simplicity. But I agree being able to change root can allow some flexibility. Feel free to create a pull request for this feature, I will merge it.