-
Notifications
You must be signed in to change notification settings - Fork 26.3k
implement numpy-like functionality isposinf, isneginf #41588
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
implement numpy-like functionality isposinf, isneginf #41588
Conversation
💊 CI failures summary and remediationsAs of commit 398ed32 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 38 times. |
03e17f0 to
d6c6d4f
Compare
d6c6d4f to
63470b4
Compare
|
@mruberry The PR implements both |
|
Please kindly let me know whether I need to update the logic for supporting XLA, or skip the XLA tests with |
|
cc: @JackCaoG for the XLA failure. |
b91e402 to
c1bde68
Compare
|
Apologizes for triggering requesting reviews from code owners due to the inappropriate operations of git merge. I will use this command in further contributions. |
|
The CI checks all passed thanks to ailzhang's kind suggestions regarding XLA, and this PR is ready for review, |
mruberry
left a comment
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.
Reviewed the user of TensorIterator. You're on the right path but I suggest using a trick for bool and integer cases that will greatly simplify your code. I also have a question about how the bfloat16 dispatch path works since I don't think the cpp standard supports numeric limits for bfloat16? Maybe I'm mistaken.
The tests look good. I'm sorry I wrote something confusing about the documentation in our last discussion. I added more context. Please let me know if it makes sense now.
@mruberry I cannot thank you more for the kind help and mentoring during this PR, the kernels have been simplified significantly by adopting the suggested approach. NumPy supports users in passing In [1]: import numpy as np
In [2]: x = np.array([float('inf'), float('inf')])
In [3]: y = np.array([10, 20])
In [4]: np.isposinf(x, out=y)
Out[4]: array([1, 1])
In [5]: y
Out[5]: array([1, 1])Thus I kept I also experimented with dispatching pytorch/c10/util/BFloat16-inl.h Lines 203 to 208 in c0bfa45
This may be the reason why bfloat16 tests are passed.
|
mruberry
left a comment
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.
Cool! Looks good to me!
Thank you for working on this, @RockingJavaBean, and being willing to iterate on it. Please let me know if you'd like to implement another PyTorch function.
facebook-github-bot
left a comment
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.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mruberry sure, I love to implement another PyTorch function. |
Great! Since you just did a pair of unary functions, how about a pair of binary functions? numpy.maximum and numpy.minimum are another pair of functions whose implementations will have some similarities with the work you've done here, like using TensorIterator, but will require writing binary kernels, handling complex values, and dealing with different output dtypes. Do those sound interesting? |
|
@mruberry Definitely, they sound really interesting, and thanks for the tips regarding implementation. |
|
This has been merged! Congratulations, @RockingJavaBean! |
Related #38349
Numpy-like functionalities
isposinfandisneginfare implemented.Test-Plan: