fix pos encoding error + target prefixing error in LM decoding #2099
Conversation
| x = x.permute(perm).contiguous() | ||
| x = x.permute(perm) |
There was a problem hiding this comment.
Is this change necessary?
If there is a break in terms of memory layout, shouldn't we fix it at the nearest place possible?
Leave a tensor variable to stay broken rather than fix it right away feels worrying.
There was a problem hiding this comment.
Does it matter that the operation is not done at this point ? It will be performed in the next lines. I can remove it but I will have to perform the second contiguous operation in this function anyhow.
The tile function is/should not be called more than once per vector and per line of inference.
There was a problem hiding this comment.
No, it does not matter actually. But I personally prefer the array to stays in good condition at any point in the function if such change does not have any benefits in terms of performance.
Here is a nice explanation for contiguous or non-contiguous.
onmt/translate/translator.py
Outdated
| log_probs = fn_map_state(log_probs, dim=1) | ||
| if fn_map_state is not None: |
There was a problem hiding this comment.
This does not look good.
There was a problem hiding this comment.
Can you be more specific ?
There was a problem hiding this comment.
You could not use the variable as a function and then check if it's None or not. Also, I see there was another call for the same process a few lines before which seems redundant.
But now, it's all good.
No description provided.