d2l-en

d2l-en copied to clipboard

The TF implementation of d2l.predict_seq2seq in https://github.com/d2l-ai/d2l-en/pull/1768/files?file-filters%5B%5D=.md#diff-dbae7acee5140a9e76359207c8a1b718efcc82d4e7fbd194d6036ab0ee8e2130R883 removes devices argument with a note "We don't need the device argument in TF as TF uses available device automatically."

@biswajitsahoo1111 and @terrytangyuan, does it hurt if we force to specify device like what we do in the mxnet and pytorch implementations? One benefit is that the same d2l.predict_seq2seq function will share the same interface (same list of arguments, all passing device) to allow us to combine many code blocks to avoid redundancy.

Yes, I agree we should be consistent. @biswajitsahoo1111 Any special reason to not do that? I think at least we could make sure the function signature is consistent.

@astonzhang @terrytangyuan There is no special reason behind that choice. I probably ran into some error while using it so I removed it for personal convenience. I will look into it and try to incorporate it for consistency. I might require @terrytangyuan's help in this regard. But first let me try and see how far I can go in resolving this. Thanks.

@astonzhang @terrytangyuan One of the simplest solutions would be to include the argument in d2l.predict_seq2seq but never use it inside the function (because we don't have to in TF). That would make the code consistent with other implementations and would not cause any errors. But would that be a good choice to make? We would just be adding a redundant argument without ever using it. Let me know of your opinions.

Thanks. As long as the current implementation is correct (@terrytangyuan please double check), I'm ok with leaving it out in the argument list for now.

Yes that's fine. This is pretty minor in my opinion.