Add note about in-place weight modification for nn.Embedding #45595
Conversation
|
CI failure doesn't seem to be my fault, so I think this is ready for review. |
mrshenli
added
module: nn
torch/nn/modules/sparse.py
Outdated
| .. note:: | ||
| When :attr:`max_norm` is not ``None``, :class:`Embedding`'s forward method will modify the | ||
| :attr:`weight` Tensor in-place, rather than setting ``Embedding.weight`` to a new Tensor. | ||
| Because of this, if you want to perform some calculation with ``Embedding.weight`` before |
There was a problem hiding this comment.
Can we be clearer about the reason for this? Maybe something like,
"Since tensors needed for gradient computations cannot be modified inplace, using the Embedding.weight tensor before calling Embedding's forward method requires cloning it when max_norm is not None. For example::"
There was a problem hiding this comment.
I updated this to be similar to your suggestion, just with a couple extra details
|
Hey @kurtamohler! This is a great, well-written fix for a confusing issue. I made a couple suggestions and I look forward to hearing your thoughts on them. |
kurtamohler
force-pushed
the
embedding-inplace-mod-doc-26596
branch
from
393dc26
to
e483241
Compare
|
@mruberry , thanks for the comments. I've made the changes you asked for, so I think it's good to go |
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Codecov Report
@@ Coverage Diff @@
## master #45595 +/- ##
=========================================
Coverage ? 68.19%
=========================================
Files ? 410
Lines ? 53232
Branches ? 0
=========================================
Hits ? 36303
Misses ? 16929
Partials ? 0
Continue to review full report at Codecov.
|
Fixes #26596