Skip to content

Add randn in onnx symbolic#12880

Closed
zrphercule wants to merge 6 commits intopytorch:masterfrom
zrphercule:fix_randn
Closed

Add randn in onnx symbolic#12880
zrphercule wants to merge 6 commits intopytorch:masterfrom
zrphercule:fix_randn

Conversation

@zrphercule
Copy link
Copy Markdown
Contributor

In this pr we added operator randn in onnx symbolic. Also, related tests are added.

Copy link
Copy Markdown
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@houseroad
Copy link
Copy Markdown
Member

import to phabricator?

@zrphercule
Copy link
Copy Markdown
Contributor Author

@houseroad Let me take a look of the failing CI

Comment thread test/onnx/test_operators.py Outdated

This comment was marked as off-topic.

This comment was marked as off-topic.

Copy link
Copy Markdown
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

You need to translate the attribute name, too:
https://github.com/onnx/onnx/blob/master/docs/Operators.md#RandomNormal
https://github.com/pytorch/pytorch/blob/master/caffe2/operators/filler_op.h#L405
Not exactly matching...

Also, no support for seed in Caffe2, we need to throw exception in such cases

@zrphercule
Copy link
Copy Markdown
Contributor Author

@houseroad Yeah, I only took care of "shape" but forgot other attributes...

@zrphercule
Copy link
Copy Markdown
Contributor Author

@houseroad review?

Comment thread caffe2/python/onnx/backend.py Outdated

This comment was marked as off-topic.

Copy link
Copy Markdown
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

Also throw exceptions in python conversion code? Otherwise, LGTM.

@zrphercule
Copy link
Copy Markdown
Contributor Author

@houseroad I have tested, and it seems even if there are extra attributes in nodes in python backend, it is okay. I will merge if once ci has been passed.

@houseroad
Copy link
Copy Markdown
Member

Interesting, it's ok to merge it if CI passes. You can import first while waiting the CI.

Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
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.

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

@zrphercule zrphercule closed this Oct 23, 2018
@zrphercule zrphercule reopened this Oct 23, 2018
Copy link
Copy Markdown
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.

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

@houseroad
Copy link
Copy Markdown
Member

If failures are just time-out and caffe2_onnx_py2_gcc_ubuntu16_04_build/test are green, you probably can just land it

@zrphercule
Copy link
Copy Markdown
Contributor Author

@houseroad I believe so. Let's land it tomorrow morning!~

Copy link
Copy Markdown
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.

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

@zrphercule zrphercule deleted the fix_randn branch October 25, 2018 22:20
@ezyang ezyang added the merged label Jun 25, 2019
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
Summary:
In this pr we added operator randn in onnx symbolic. Also, related tests are added.
Pull Request resolved: pytorch#12880

Reviewed By: houseroad

Differential Revision: D10501788

Pulled By: zrphercule

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants