Skip to content

Conversation

@hikjik
Copy link
Contributor

@hikjik hikjik commented Apr 10, 2019

This PR refer to issue #19026

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 10, 2019
@eellison eellison requested review from driazati and eellison April 11, 2019 16:17
@driazati
Copy link
Contributor

driazati commented Apr 11, 2019

Ideally we'd be able to bind the std::... math functions as custom ops, but to do that #18589 needs to land first before we enable binding custom ops to the aten:: namespace (no changes for this are necessary in this PR). This would clean up a lot of the verbose Operations that just call out to some function.

}),

Operator(
"aten::ceil(float a) -> int",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return a float (and could you change aten::floor above to also return a float)? Casting a float to an int may truncate the value if its bigger than std::numeric_limits<int64_t>::max(). It's safer to make the user explicitly agree to truncate.

@torch.jit.script
def double_floor(x):
    # type: (float) -> float
    return x - x % 1

@torch.jit.script
def int_floor(x):
    # type: (float) -> int
    return int(x - x % 1)

print(double_floor(1.2))
print(int_floor(1.2))
print(double_floor(float(5e29)))
print(int_floor(float(5e29)))  # incorrect in torchscript

Copy link
Contributor

@driazati driazati left a comment

Choose a reason for hiding this comment

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

Looks good, just one more comment before we merge this. The CI failures are probably unrelated.

test/test_jit.py Outdated
return math.pow(2.0, 2.0)

def test_pow_int():
return math.pow(2.0, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

For all these tests could you refactor them to take in an input instead of using a literal? This will let us test different values more easily.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

@driazati merged this pull request in da4ff17.

@hikjik hikjik deleted the jit/math-module-support branch April 17, 2019 07:23
facebook-github-bot pushed a commit that referenced this pull request Apr 19, 2019
Summary:
Fixes: #19253

Fixing pow(Tensor, float) is straightforward.
The breakage for pow(float, Tensor) is a bit more subtle to trigger, and fixing needs `torch.log` (`math.log` didn't work) from the newly merged #19115  (Thanks ngimel for pointing out this has landed.)
Pull Request resolved: #19324

Differential Revision: D15003531

Pulled By: ailzhang

fbshipit-source-id: 8b22138fa27a43806b82886fb3a7b557bbb5a865
zhangguanheng66 pushed a commit to zhangguanheng66/pytorch that referenced this pull request May 6, 2019
Summary:
This PR refer to issue [pytorch#19026](pytorch#19026)
Pull Request resolved: pytorch#19115

Differential Revision: D14936053

Pulled By: driazati

fbshipit-source-id: 68d5f33ced085fcb8c10ff953bc7e99df055eccc
zhangguanheng66 pushed a commit to zhangguanheng66/pytorch that referenced this pull request May 6, 2019
Summary:
Fixes: pytorch#19253

Fixing pow(Tensor, float) is straightforward.
The breakage for pow(float, Tensor) is a bit more subtle to trigger, and fixing needs `torch.log` (`math.log` didn't work) from the newly merged pytorch#19115  (Thanks ngimel for pointing out this has landed.)
Pull Request resolved: pytorch#19324

Differential Revision: D15003531

Pulled By: ailzhang

fbshipit-source-id: 8b22138fa27a43806b82886fb3a7b557bbb5a865
xiaomengy pushed a commit to xiaomengy/pytorch that referenced this pull request May 28, 2019
Summary:
Not much to say. Fixes implementation introduced here: pytorch#19115
Pull Request resolved: pytorch#21041

Differential Revision: D15528801

Pulled By: Chillee

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants