Review examples in README

[FrancescoSaverioZuppichini]

Hi Igor,

First of all, thank you for the repo, super cool and very usefull. Hovewer, I am sorry but I have to say it, all the examples in the readme use bad coding practices. Take this:

class ConvBlock(nn.Module):
    def __init__(self):
        super(ConvBlock, self).__init__()
        block = [nn.Conv2d(...)]
        block += [nn.ReLU()]
        block += [nn.BatchNorm2d(...)]
        self.block = nn.Sequential(*block)
    
    def forward(self, x):
        return self.block(x)

Why create a 1 item array every time and add it to block? It doesn't make sense, it is confusing and useless. Do this instead

class ConvBlock(nn.Module):
    def __init__(self):
        super(ConvBlock, self).__init__()
        self.block = nn.Sequential(
                        nn.Conv2d(...),  
                        nn.ReLU(), 
                        nn.BatchNorm2d(...)
)
    
    def forward(self, x):
        return self.block(x)

Cleaner and faster to code, or even better:

class ConvBlock(nn.Sequential):
    def __init__(self):
        super().__init__( nn.Conv2d(...),  
                        nn.ReLU(), 
                        nn.BatchNorm2d(...))

No need to write the forward method.

Hope it helps and I hope to see better and better code in the future :)

[IgorSusmelj]

Hi Francesco,

you're right. The code there is not nice. It was a leftover to showcase that what you pass to sequential can be broken up into parts (items of a list). But in this example it makes no sense. I will replace it with the first option you proposed.

The second option is very interesting. I haven't seen this approach yet. For simple blocks this works fine and reduces the code. I wouldn't use this approach myself since it makes it harder for me to read and understand the code. I'm used to seeing models and building blocks as modules with forward passes.

However, I will leave the issue as always open to let others discover this :)

[lartpang]

Furthermore, we can also write the code like this:

class ConvBlock(nn.Module):
    def __init__(self):
        super(ConvBlock, self).__init__()
        self.block = nn.Sequential()
        self.block.add_module(name="conv", module=nn.Conv2d(...))
        self.block.add_module(name="relu", module=nn.ReLU())
        self.block.add_module(name="bn", module=nn.BatchNorm2d(...))

    def forward(self, x):
        return self.block(x)

The method add_module is the method of parent class Module of the class Sequential, it need to specify the name of the module..

It is similar to the following code, but it's more flexible:

class ConvBlock(nn.Module):
    def __init__(self):
        super(ConvBlock, self).__init__()
        self.block = nn.Sequential(
            OrderedDict(
                [
                    ("conv", nn.Conv2d(...)),
                    ("relu", nn.ReLU()),
                    ("bn", nn.BatchNorm2d(...)),
                ]
            )
        )

    def forward(self, x):
        return self.block(x)

[FrancescoSaverioZuppichini]

Yep sure, I am not a fan of the .add_module method, doesn't seem very polish to me to call if inside the class. The best way is to subclass nn.Sequential IMHO. I know good software engineer practices (such as inheritance) and not used in the machine learning community because (between me and you) most of them are just bad codes.