hunkim / PyTorchZeroToAll

Simple PyTorch Tutorials Zero to ALL!
http://bit.ly/PyTorchZeroAll
3.89k stars 1.2k forks source link

Call for comments for seq2seq and charrnn #7

Open hunkim opened 6 years ago

hunkim commented 6 years ago

Could you please check if this code is easy/correct enough to show the basic concepts?

https://github.com/hunkim/PyTorchZeroToAll/blob/master/13_1_seq2seq.py https://github.com/hunkim/PyTorchZeroToAll/blob/master/12_5_char_rnn.py

@yunjey @kkweon, thanks in advance!

kkweon commented 6 years ago

As of now, it is easy to understand. :+1: :+1: :+1:

But I think there's still room for improvement.

1. Combine multiple global expressions into one main function

Instead of writing everything in a procedural way

def some_function():
    pass

expression1

expression2

blah blah

Le'ts do this!

def some_function():
    pass

def main():
    expression1
    expression2
    blah_blah

if __name__ == "__main__":
    main()

because it's hard to differentiate from the function definitions. It seemed quite confusing to me even though it was placed at the bottom.

2. Let's add comments with type annotations (and avoid bad names)

str is a python keyword and I would still avoid it since the pylint will nag about this. And I don't want to write a custom lintrc for that.

So, instead of this

def str2tensor(str, eos=False):
    tensor = [ord(c) for c in str]
    if eos:
        tensor.append(EOS_token)
    tensor = torch.LongTensor(tensor)

    # Do before wrapping with variable
    if torch.cuda.is_available():
        tensor = tensor.cuda()

    return Variable(tensor)

Let's do this. Plus, I am greatly in favor of type annotation though others may have different opinions.

def str2tensor(message: str, eos: bool=False) -> Variable:
    """Converts message to a tensor

    Args:
        message (str): Message string
        eos (bool, optional): Append `EOS_Token`

    Returns:
        Variable: The result is a tensor `torch.autograd.Variable` object
    """
    tensor = [ord(c) for c in message]
    if eos:
        tensor.append(EOS_token)
    tensor = torch.LongTensor(tensor)

    # Do before wrapping with variable
    if torch.cuda.is_available():
        tensor = tensor.cuda()

    return Variable(tensor)

Benefits of Type annotation

hunkim commented 6 years ago

@kkweon Thanks. I try to make the main, but in this case, we need to pass encoder and decoder for train and translate functions. Do you think it's OK?

kkweon commented 6 years ago

Yes, explicit > implicit. Wasn't it one of your favorite quotes as well :wink: ?

hunkim commented 6 years ago

@kkweon, also need to pass criterion as an argument. :-)