Skip to content

Warn about floor_divide performing incorrect rounding#50281

Closed
peterbell10 wants to merge 35 commits intogh/peterbell10/37/basefrom
gh/peterbell10/37/head
Closed

Warn about floor_divide performing incorrect rounding#50281
peterbell10 wants to merge 35 commits intogh/peterbell10/37/basefrom
gh/peterbell10/37/head

Conversation

@peterbell10
Copy link
Copy Markdown
Collaborator

@peterbell10 peterbell10 commented Jan 8, 2021 •
edited
Loading

Stack from ghstack:

Differential Revision: D26257855

This was referenced Jan 8, 2021
Closed
Closed
peterbell10 added a commit that referenced this pull request Jan 8, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
d84136a 
ghstack-source-id: f444659
Pull Request resolved: #50281
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Jan 8, 2021
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Jan 8, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit cd325b9 (more details on the Dr. CI page):

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

@peterbell10 peterbell10 requested a review from mruberry January 8, 2021 17:05
peterbell10 added a commit that referenced this pull request Jan 8, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
ae3bbf1 
ghstack-source-id: ba35aa6
Pull Request resolved: #50281
peterbell10 added a commit that referenced this pull request Jan 8, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
3e72a81 
ghstack-source-id: 0d88aa6
Pull Request resolved: #50281
peterbell10 added a commit that referenced this pull request Jan 9, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
09ac1ef 
ghstack-source-id: 00f3145
Pull Request resolved: #50281
This was referenced Jan 12, 2021
Closed
Merged
Closed
mruberry pushed a commit to mruberry/pytorch that referenced this pull request Feb 4, 2021
@peterbell10 @facebook-github-bot
Warn about floor_divide performing incorrect rounding (pytorch#50281)
9f09b27 
Summary: Pull Request resolved: pytorch#50281

Test Plan: Imported from OSS

Reviewed By: ngimel

Differential Revision: D26257855

Pulled By: mruberry

fbshipit-source-id: 3c4a91f97fe99e7f0905212f8a9e188e28130b57
@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented Feb 4, 2021

Internal updates reflected in: #51745 (comment)

@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented Feb 4, 2021 •
edited
Loading

Hey @peterbell10, take a look at the CI on #51745, which is this PR rebased and with a docs fix (deprecated -> warning).

It seems like there's some new issue with the ONNX testing where it's being passed a div signature it doesn't understand. I'm going to investigate more now.

Update: probably a logical merge conflict with 6d47e2c? Looks like we'll have to add an ONNX export for the new div operation. Luckily that shouldn't be too hard. Ping me about it if you have any questions and we'll get this merged quickly, and then we'll cherrypick it into the 1.8 branch.

@peterbell10 peterbell10 mentioned this pull request Feb 5, 2021
Closed
peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Feb 5, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
96f7a1b 
ghstack-source-id: 982727f
Pull Request resolved: pytorch#50281
@peterbell10
Update on "Warn about floor_divide performing incorrect rounding"
25081f5 
Differential Revision: [D26257855](https://our.internmc.facebook.com/intern/diff/D26257855)

[ghstack-poisoned]
@peterbell10
Update on "Warn about floor_divide performing incorrect rounding"
43dd165 
Differential Revision: [D26257855](https://our.internmc.facebook.com/intern/diff/D26257855)

[ghstack-poisoned]
@peterbell10
Update on "Warn about floor_divide performing incorrect rounding"
227d10c 
Differential Revision: [D26257855](https://our.internmc.facebook.com/intern/diff/D26257855)

[ghstack-poisoned]
@peterbell10
Update on "Warn about floor_divide performing incorrect rounding"
f5d0cbb 
Differential Revision: [D26257855](https://our.internmc.facebook.com/intern/diff/D26257855)

[ghstack-poisoned]
@peterbell10
Update on "Warn about floor_divide performing incorrect rounding"
0174d9d 
Differential Revision: [D26257855](https://our.internmc.facebook.com/intern/diff/D26257855)

[ghstack-poisoned]
@peterbell10
Copy link
Copy Markdown
Collaborator Author

peterbell10 commented Feb 7, 2021

@mruberry PTAL. Rebased and implemented rounding_mode argument in the ONNX export. That fixes the ONNX test failures.

@mruberry
Copy link
Copy Markdown
Collaborator

mruberry commented Feb 8, 2021

fyi: the pytorch_doc_test failure was in the base (and has since been addressed). It's not an issue with this PR.

mruberry
mruberry reviewed Feb 8, 2021
View reviewed changes
Comment thread aten/src/ATen/native/BinaryOps.cpp
}

Tensor& floor_divide_out(Tensor& result, const Tensor& self, const Tensor& other) {
TORCH_WARN_ONCE(
Copy link
Copy Markdown
Collaborator

@mruberry mruberry Feb 8, 2021

Choose a reason for hiding this comment

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

Take a look at https://github.com/pytorch/pytorch/pull/51745/files. This warning got a slight update and needs to be replicated for floor_divide(). You'll have to verify manually that the warning appears for the function, method, inplace, and out= variants.

mruberry
mruberry reviewed Feb 8, 2021
View reviewed changes
Comment thread torch/onnx/symbolic_opset10.py
# release on 04/24/19


def div(g, self, other, *args):
Copy link
Copy Markdown
Collaborator

@mruberry mruberry Feb 8, 2021

Choose a reason for hiding this comment

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

The ONNX changes need a test, too. See here:

pytorch/test/onnx/test_pytorch_onnx_onnxruntime.py

Line 1308 in fcf8b71

def test_floor_div(self):

The test does not need to be extensive, it just needs to validate that the call works correctly.

mruberry
mruberry reviewed Feb 8, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @peterbell10; I'll start testing this internally. I made two notes:

  • check the updated warning message requirements and validate manually that the warning message is thrown as expected (see inline comment)
  • add tests sanity checking the ONNX behavior (see inline comment for a code reference)

peterbell10 added a commit to peterbell10/pytorch that referenced this pull request Feb 8, 2021
@peterbell10
Warn about floor_divide performing incorrect rounding
4291619 
ghstack-source-id: 9065bd9
Pull Request resolved: pytorch#50281
@peterbell10
Copy link
Copy Markdown
Collaborator Author

peterbell10 commented Feb 8, 2021

@mruberry added onnx tests, ensured warning was included on both overloads and rebased to fix the doc issue (although there seems to be a new ROCm issue now).

This was referenced Feb 9, 2021
Closed
Closed
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Feb 10, 2021

@mruberry merged this pull request in 594a66d.

@facebook-github-bot facebook-github-bot deleted the gh/peterbell10/37/head branch February 13, 2021 15:18
xsacha pushed a commit to xsacha/pytorch that referenced this pull request Mar 31, 2021
Warn about floor_divide performing incorrect rounding (pytorch#50281) (…
39e4cce 
…pytorch#50281)

Summary:
Pull Request resolved: pytorch#50281

Pull Request resolved: pytorch#51745

Test Plan: Imported from OSS

Reviewed By: ngimel

Pulled By: mruberry

Differential Revision: D26257855

fbshipit-source-id: e5d497cf07b0c746838ed081c5d0e82fb4cb701b
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@facebook-github-bot
Warn about floor_divide performing incorrect rounding (pytorch#50281) (…
c4a2508 
…pytorch#50281)

Summary:
Pull Request resolved: pytorch#50281

Pull Request resolved: pytorch#51745

Test Plan: Imported from OSS

Reviewed By: ngimel

Pulled By: mruberry

Differential Revision: D26257855

fbshipit-source-id: e5d497cf07b0c746838ed081c5d0e82fb4cb701b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mruberry mruberry mruberry approved these changes

Assignees

No one assigned

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@peterbell10 @facebook-github-bot @mruberry @pytorchbot