Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nll_loss] Avoid unnecessary type casts #86086

Closed
wants to merge 2 commits into from

Conversation

crcrpar
Copy link
Collaborator

@crcrpar crcrpar commented Oct 3, 2022

follow-up #85395

AT_DISPATCH_NLL_LOSS_INDEX_TYPES should not be removed in favor of #59765 and there's a testcase

def test_nll_loss_byte_target_matches_long(self, device):

Besides the dispatcher, I wanted to sanity check int64_t ignore_index because int64_t can be inappropriate considering that target can be Byte. However, given that the default value is -100 as in

- func: nll_loss(Tensor self, Tensor target, Tensor? weight=None, int reduction=Mean, int ignore_index=-100) -> Tensor
it's not easy to add a check while keeping the backward compatibility. Thus I decided to not add a check.

cc @lezcano @t-vi

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 3, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86086

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures, 1 Pending

As of commit 3d6bc2c:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link
Contributor

/easycla

As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details.

This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 4, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@crcrpar crcrpar marked this pull request as ready for review October 6, 2022 04:24
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
Signed-off-by: Masaki Kozuki <mkozuki@nvidia.com>
@bdhirsh bdhirsh requested a review from lezcano October 6, 2022 19:46
@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 6, 2022
Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thank you!

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 6, 2022
@crcrpar
Copy link
Collaborator Author

crcrpar commented Oct 6, 2022

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@github-actions
Copy link

github-actions bot commented Oct 7, 2022

Hey @crcrpar.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@crcrpar crcrpar deleted the nll_followup branch October 7, 2022 00:25
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
Summary:
follow-up #85395

`AT_DISPATCH_NLL_LOSS_INDEX_TYPES` should not be removed in favor of #59765 and there's a testcase https://github.com/pytorch/pytorch/blob/99ca25e6eb8299f31824bdbaf62f16f8a8db458d/test/test_nn.py#L16832

Besides the dispatcher, I wanted to sanity check `int64_t ignore_index` because `int64_t` can be inappropriate considering that `target` can be `Byte`. However, given that the default value is -100 as in https://github.com/pytorch/pytorch/blob/0a75c42f36c0e50a22c06fa65478df53d7d420c4/aten/src/ATen/native/native_functions.yaml#L9949 it's not easy to add a check while keeping the backward compatibility. Thus I decided to not add a check.

cc lezcano t-vi

Pull Request resolved: #86086
Approved by: https://github.com/lezcano

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/a2419638373071c74692c9fe5996c69ef509f581

Reviewed By: seemethere

Differential Revision: D40167225

Pulled By: seemethere

fbshipit-source-id: ae031f061a88ef90d38b48f56b91bd17ae5c0e2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request cla signed Merged open source release notes: cuda release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants