Add the appropriate check on div_value to the cpp frontend #114671
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/114671
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3c8f5c9 with merge base 373f206 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
mikaylagawarecki
added
the
triaged
There was a problem hiding this comment.
Looking at this again, it seems like there is an inconsistency between the Python and C++ API here
In this case,
int64_t hsz = options.in_features() / static_cast<int64_t>(std::pow(options.div_value(), (i + 1)));will be
int64_t hsz = 16 / static_cast<int64_t>(std::pow(options.div_value(), (i + 1)));
which gives 16/0
on the other hand in the python API the corresponding line
pytorch/torch/nn/modules/adaptive.py
Line 158 in 73a661a
would give
int(16//0.25 ** 1) = 64
I did not notice that the issue used 0 for the div_value in Python rather than 0.25 (which was used for C++), which does not error
import torch
m = torch.nn.AdaptiveLogSoftmaxWithLoss(1,3,(1,),div_value=0.25)Instead of a check on the denominator, maybe we need to do
static_cast<int64_t>(std::floor(options.in_features() / std::pow(options.div_value(), (i + 1))));And also a separate TORCH_CHECK that div_value is not 0 (iiuc that is the only case where std::pow(div_value, i) will be 0)
Apologies for the churn! Let me know if you agree with the above analysis
There was a problem hiding this comment.
Sorry, It was my mistake, I agree with you very much, and have fixed it again, please have a look at it again.
Thank you.
There was a problem hiding this comment.
LGTM!
cc @albanD on whether this is considered a bugfix or BC-breaking
| { | ||
| // test div_value | ||
| auto options = | ||
| AdaptiveLogSoftmaxWithLossOptions(16, 20, {4, 10, 15}).div_value(0.); |
There was a problem hiding this comment.
nit: could we also check that .div_value(0.25) doesn't error here
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 11 jobs have failed, first few of them are: pull / linux-focal-py3.11-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-jammy-py3.8-gcc11 / test (default, 1, 3, linux.2xlarge) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -i |
Merge failedReason: 12 jobs have failed, first few of them are: pull / linux-focal-py3.11-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.11-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (default, 1, 3, linux.2xlarge), pull / linux-focal-py3.8-clang10 / test (crossref, 1, 2, linux.2xlarge), pull / linux-jammy-py3.8-gcc11 / test (default, 1, 3, linux.2xlarge) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 0 checks: Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
…14671) Fixes pytorch#114334 As the title stated. Pull Request resolved: pytorch#114671 Approved by: https://github.com/mikaylagawarecki
4 participants
Fixes #114334
As the title stated.