-
Notifications
You must be signed in to change notification settings - Fork 65
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.
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
added view_as
#351
added view_as
#351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @k223kim! Cool stuff! I made a couple minor comments
Forgot to add -- yes, a test with an opinfo would be great! It doesn't have to be especially complicated |
Hi @mruberry! Thanks for the awesome review! Just some questions regarding this PR. I have added a test case for
Do you think this is necessary? |
Nope, don't need to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before submitting
What does this PR do?
Fixes #328. I am not sure if I should include the test case for
view_as
opinfos.py
(cause there wasn't one forview
, maybe it is similar toreshape
?). Also, I addedview_as
to prims, although I am not sure if this is necessary. Let me know how I can improve!PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
👊