Skip to content

[jit] math module support: gcd, copysign, erf, erfc, expm1, fabs, gamma, lgamma #19707

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.

Sign up for GitHub

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

Jump to bottom
Closed
eugenekoran wants to merge 13 commits into from
Closed

[jit] math module support: gcd, copysign, erf, erfc, expm1, fabs, gamma, lgamma #19707

eugenekoran wants to merge 13 commits into from

Conversation

@eugenekoran
Copy link
Contributor

@eugenekoran eugenekoran commented Apr 25, 2019 •
edited
Loading

@eellison @driazati Refer to issue #19026

@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Apr 25, 2019
@eellison
Copy link
Contributor

eellison commented May 9, 2019

@pytorchbot retest this please

@eellison
Copy link
Contributor

eellison commented May 9, 2019

@pytorchbot rebase this please

@eugenekoran
Copy link
Contributor Author

eugenekoran commented May 9, 2019

Ok, I see tests fail in python 2 because there is no gcd method in math module.

Seems like _builtin_table is used for generating documentation only. I am not sure how to find a work-around for it.

16:28:12 + python -c 'import torch; print(torch.__config__.show())'
16:28:21 Traceback (most recent call last):
16:28:21   File "<string>", line 1, in <module>
16:28:21   File "/var/lib/jenkins/.local/lib/python2.7/site-packages/torch/__init__.py", line 291, in <module>
16:28:21     import torch.jit
16:28:21   File "/var/lib/jenkins/.local/lib/python2.7/site-packages/torch/jit/__init__.py", line 1762, in <module>
16:28:21     _register_builtin(len, 'aten::len')
16:28:21   File "/var/lib/jenkins/.local/lib/python2.7/site-packages/torch/jit/__init__.py", line 1755, in _register_builtin
16:28:21     _get_builtin_table()[id(fn)] = op
16:28:21   File "/var/lib/jenkins/.local/lib/python2.7/site-packages/torch/jit/__init__.py", line 1732, in _get_builtin_table
16:28:21     _builtin_table[id(math.gcd)] = "aten::gcd"
16:28:21 AttributeError: 'module' object has no attribute 'gcd'

@eellison
Copy link
Contributor

eellison commented May 9, 2019

@eugenekoran you can guard math.gcd with a if not PY2 check

@eugenekoran
Copy link
Contributor Author

eugenekoran commented May 10, 2019

@eellison Would you mind taking a look?

eellison
eellison approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

Looks great thanks for doing this!

I added a couple comments to the tests, but they're nits since it's test code

test/test_jit.py Outdated
# map functions to the test values from their domains
default_inputs = [1, 10, 0, -1, -1.5, 5.0, 1.5]
func_inputs = {func: default_inputs for func in ['erf', 'erfc', 'expm1', 'fabs']}
func_inputs.update(
Copy link
Contributor

@eellison eellison May 10, 2019

Choose a reason for hiding this comment

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

nit: would be a little more readable & not duplicate the default inputs if you had something like -
zero_not_def = ['gamma', 'lgamma']
and then below you can add a line like:
if func in zero_not_def and scalar == 0: continue

test/test_jit.py Outdated
# type: (float, float) -> float
return math.copysign(x, y)

for inputs in [(3, 5), (3, -5), (-3, 5), (-3, -5), (3, 0), (0, 3)]:
Copy link
Contributor

@eellison eellison May 10, 2019

Choose a reason for hiding this comment

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

nit: to avoid duplicating the list of scalars, you can define all inputs as floats, and then cast the int inputs.

@eellison
Copy link
Contributor

eellison commented May 10, 2019

Looks like you have a lint failure:
./test/test_jit.py:5860:100: E241 multiple spaces after ','

This one's pretty easy to fix. For future reference: https://github.com/pytorch/pytorch/wiki/Lint-as-you-type

facebook-github-bot
facebook-github-bot reviewed May 10, 2019
View reviewed changes
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.

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 13, 2019

@eellison merged this pull request in 5f7ef09.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@eellison eellison eellison approved these changes

+1 more reviewer

@driazati driazati driazati approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

6 participants

@eugenekoran @eellison @facebook-github-bot @driazati @ezyang @pytorchbot