[jit] Implements divmod function #20979

pandeykartikey · 2019-05-27T05:34:44Z

This PR refer to issue #18627

wanchaol

Also, please add overloads for floating point numbers, this is now only working for integers which is not complete, consider using the micros (like DEFINE_BINARY_OP, etc) instead of adding it explicitly.

wanchaol · 2019-05-28T05:15:10Z

torch/csrc/jit/register_prim_ops.cpp

please put this into aten namespace, everything that has schema should go to aten instead of prim, prim is reserved for unschematized operator, other ops like abs did it wrong and we will fix it in the future.

pandeykartikey · 2019-05-28T12:08:50Z

Hi @wanchaol the implementation of floating point and integer variation of divmod are very different. So, I have added it explicitly.
There was also no micro which handles two outputs. Shall I add one micro for that too?

wanchaol

looks good on the implementation, you don't need to add the micro for it considering that it is a rare use case that returns two results, making them separately sounds fine. it would be good if you can add mixed operand type overloads as well :)

wanchaol · 2019-05-28T19:47:22Z

torch/csrc/jit/register_prim_ops.cpp

what about mixed operand types? like (int, float) and (float, int)? Can you also add these two? we should apply binary arithmetic operators rules for those use cases according to the python documentation

Apply binary arithmetic operators rules would it mean calculating quotient first using / and then calculating mod using it?

@pandeykartikey Yep that's correct. In general results returned here should match python divmod results in all combinations of operand types. Given there will be similar combo of them, it's the best to be generated (similar to aten::log) instead of writing each Operator separately with similar implementation.

driazati · 2019-05-28T22:01:32Z

test/test_jit.py

Can you also check with some negative inputs? Python and C++ differ in their mod with negative numbers, we want to make sure it matches the Python behavior

Also need to error check for 0 input

ailzhang · 2019-06-05T17:01:36Z

@pandeykartikey would you mind resolving the comments & conflicts?

pandeykartikey · 2019-06-05T17:24:24Z

@ailzhang I was waiting for a reply by @wanchaol on a doubt about binary arithmetic on mixed operands. All the others comments have been resolved in the previous commit.

ezyang · 2019-06-06T16:56:10Z

@wanchaol could you please follow up on this PR?

needs response

pandeykartikey · 2019-06-07T03:49:30Z

@ailzhang @wanchaol @driazati This PR seems complete from my side would you mind taking a look.

wanchaol

Thanks for adding the mix operand type overloads! The PR looks good to me, but it needs to rebase with the master since there're some file conflicts recently. Also have some small nits.

wanchaol · 2019-06-07T05:40:17Z

test/test_jit.py

can't you just use self.checkScript(func_int, (1024, 0)) here and other exception cases?

wanchaol · 2019-06-07T05:41:31Z

torch/csrc/jit/register_prim_ops.cpp

\ in this macro are not aligned, can you indent and align them?

pandeykartikey · 2019-06-07T06:56:35Z

@wanchaol I have rebased the branch on master and made the other fixes.

wanchaol · 2019-06-07T07:21:15Z

@pandeykartikey do you mind fix the clang-tidy linting issue?

Traceback (most recent call last):
  File "tools/clang_tidy.py", line 295, in <module>
    main()
  File "tools/clang_tidy.py", line 291, in main
    print(run_clang_tidy(options, line_filters, files))
  File "tools/clang_tidy.py", line 189, in run_clang_tidy
    raise RuntimeError(message.format(output))
RuntimeError: Found clang-diagnostic-errors in clang-tidy output: /home/travis/build/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp:2407:9: error: expected ')' [clang-diagnostic-error]
        "aten::divmod(int x, int y) -> (int, int)",
        ^
/home/travis/build/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp:2406:13: note: to match this '('
    Operator(
            ^
/home/travis/build/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp:2407:9: error: expected unqualified-id [clang-diagnostic-error]
        "aten::divmod(int x, int y) -> (int, int)",
        ^
/home/travis/build/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp:2472:13: error: use of undeclared identifier 'simpleClassTypeArg' [clang-diagnostic-error]
           !simpleClassTypeArg(schema_args[0], class_type) ||
            ^
/home/travis/build/pytorch/pytorch/torch/csrc/jit/register_prim_ops.cpp:2473:13: error: use of undeclared identifier 'simpleClassTypeArg' [clang-diagnostic-error]
           !simpleClassTypeArg(schema_args[1], class_type) ||

Maybe worth setting it up locally if you wish, see contributing doc

pandeykartikey · 2019-06-07T12:06:12Z

@wanchaol Fixed the linting issue.

zou3519

Some of the tests are failing (click one of the red 'X's). We should fix those before landing.

Jun 07 13:09:48 ======================================================================
Jun 07 13:09:48 ERROR: test_divmod (__main__.TestScript)
Jun 07 13:09:48 ----------------------------------------------------------------------
Jun 07 13:09:48 Traceback (most recent call last):
Jun 07 13:09:48   File "test_jit.py", line 5666, in test_divmod
Jun 07 13:09:48     self.checkScriptRaisesRegex(func_int, (1024, 0), RuntimeError, "division by 0")
Jun 07 13:09:48   File "test_jit.py", line 2898, in checkScriptRaisesRegex
Jun 07 13:09:48     script(*inputs)
Jun 07 13:09:48   File "test_jit.py", line 5638, in func_int
Jun 07 13:09:48     return divmod(a, b)
Jun 07 13:09:48 ZeroDivisionError: integer division or modulo by zero
Jun 07 13:09:48 
Jun 07 13:09:48 ----------------------------------------------------------------------
Jun 07 13:09:48 Ran 1934 tests in 585.133s
Jun 07 13:09:48 
Jun 07 13:09:48 FAILED (errors=1, skipped=45, expected failures=1)
Jun 07 13:09:49 Traceback (most recent call last):
Jun 07 13:09:49   File "test/run_test.py", line 449, in <module>
Jun 07 13:09:49     main()
Jun 07 13:09:49   File "test/run_test.py", line 441, in main
Jun 07 13:09:49     raise RuntimeError(message)
Jun 07 13:09:49 RuntimeError: test_jit failed!
Jun 07 13:09:49 + cleanup
Jun 07 13:09:49 + retcode=1
Jun 07 13:09:49 + set +x

pandeykartikey · 2019-06-10T03:33:17Z

Hi @wanchaol I switched back to my previous implementation of zero division error because the checkScriptWithRegex was not catching the error in the case of division by zero it was just throwing it and hence the previous tests didn't pass.

facebook-github-bot

@wanchaol has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

stale

Summary: This PR refer to issue #18627 Pull Request resolved: pytorch/pytorch#20979 Differential Revision: D15743929 Pulled By: wanchaol fbshipit-source-id: 967fc3fd519501e427176e10b112c8be1390540b

facebook-github-bot · 2019-06-10T23:36:03Z

@wanchaol merged this pull request in 2378c12.

pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 27, 2019

pandeykartikey force-pushed the jit-divmod branch from ddb87ca to 603a834 Compare May 27, 2019 06:11

pytorchbot added the module: internals Related to internal abstractions in c10 and ATen label May 27, 2019

pandeykartikey force-pushed the jit-divmod branch from 603a834 to 7cab633 Compare May 27, 2019 06:47

pandeykartikey marked this pull request as ready for review May 27, 2019 06:48

pandeykartikey mentioned this pull request May 27, 2019

[jit] Python built-in function support #18627

Closed

76 tasks

pandeykartikey force-pushed the jit-divmod branch from d63408f to 8bd2d01 Compare May 27, 2019 18:28

wanchaol previously requested changes May 28, 2019

View reviewed changes

wanchaol reviewed May 28, 2019

View reviewed changes

driazati reviewed May 28, 2019

View reviewed changes

pandeykartikey force-pushed the jit-divmod branch 2 times, most recently from 3c622c1 to 4a7d8d5 Compare May 29, 2019 12:22

ezyang added the open source label Jun 5, 2019

pandeykartikey force-pushed the jit-divmod branch 3 times, most recently from bf71afb to 8c37b6d Compare June 6, 2019 16:37

ezyang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 6, 2019

wanchaol approved these changes Jun 7, 2019

View reviewed changes

pandeykartikey added 6 commits June 7, 2019 12:16

Adds divmod function

2d48a02

Fix errors for Mac build

6019ec4

Overloads divmod for floating point values

c6108cc

Adds support for negative values in divmod

f331a8a

Adds mixed operator support for divmod

86f534c

Fixes minor issues with linting

33694bc

pandeykartikey force-pushed the jit-divmod branch 2 times, most recently from 4bf1870 to 35ef1c9 Compare June 7, 2019 06:55

Fixes merge conflicts

b4d7f87

pandeykartikey force-pushed the jit-divmod branch from 35ef1c9 to b4d7f87 Compare June 7, 2019 06:58

fix linting issues

d302616

zou3519 previously requested changes Jun 7, 2019

View reviewed changes

pytorchbot added module: nccl Problems related to nccl support module: onnx Related to torch.onnx module: third_party labels Jun 8, 2019

pandeykartikey force-pushed the jit-divmod branch 5 times, most recently from b26566b to 913e552 Compare June 8, 2019 15:33

Fixes error message

913e552

facebook-github-bot reviewed Jun 10, 2019

View reviewed changes

facebook-github-bot closed this in 2378c12 Jun 10, 2019

facebook-github-bot added the merged label Jun 10, 2019

mruberry added the Merged label Oct 28, 2020

thomasahle mentioned this pull request Dec 14, 2022

Support divmod for tensors #90820

Open

[jit] Implements divmod function #20979

[jit] Implements divmod function #20979

Uh oh!

Conversation

pandeykartikey commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wanchaol left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol May 28, 2019

Choose a reason for hiding this comment

Uh oh!

pandeykartikey commented May 28, 2019

Uh oh!

wanchaol left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pandeykartikey May 29, 2019

Choose a reason for hiding this comment

Uh oh!

ailzhang Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

driazati May 28, 2019

Choose a reason for hiding this comment

Uh oh!

eellison May 28, 2019

Choose a reason for hiding this comment

Uh oh!

ailzhang commented Jun 5, 2019

Uh oh!

pandeykartikey commented Jun 5, 2019

Uh oh!

ezyang commented Jun 6, 2019

Uh oh!

pandeykartikey commented Jun 7, 2019

Uh oh!

wanchaol left a comment

Choose a reason for hiding this comment

Uh oh!

wanchaol Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

wanchaol Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

pandeykartikey commented Jun 7, 2019

Uh oh!

wanchaol commented Jun 7, 2019

Uh oh!

pandeykartikey commented Jun 7, 2019

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

pandeykartikey commented Jun 10, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

pandeykartikey commented May 27, 2019 •

edited

Loading

wanchaol May 28, 2019 •

edited

Loading

ailzhang Jun 5, 2019 •

edited

Loading