Implementing NumPy-like function torch.signbit() #41589

Kiyosora · 2020-07-17T12:26:21Z

Related with NumPy-like Functionality Request Rollup #38349
Implementing the NumPy-like function torch.signbit() .

dr-ci · 2020-07-17T12:27:43Z

💊 CI failures summary and remediations

As of commit 9db39c6 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

3/3 broken upstream at merge base c18223f on Jul 29 from 4:30pm to 7:01pm PDT (1 commit; c18223f - fe4f19e)

🚧 3 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_windows_vs2019_py36_cuda10.1_test2 on Jul 29 from 4:30pm to 7:01pm PDT (1 commit; c18223f - fe4f19e)
- 🔁 rerun
pytorch_windows_vs2019_py36_cuda10.1_test1 on Jul 29 from 4:30pm to 7:01pm PDT (1 commit; c18223f - fe4f19e)
- 🔁 rerun
pytorch_windows_vs2019_py36_cuda10.1_test-jit-profiling-tests on Jul 29 from 4:30pm to 7:01pm PDT (1 commit; c18223f - fe4f19e)
- 🔁 rerun

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.

See how this bot performed.

This comment has been revised 40 times.

Kiyosora · 2020-07-20T01:40:30Z

Hi, @mruberry ! I implemented the torch.signbit() method and it's now ready to be reviewed. It seems that there is an XLA-related error occured in the CI test. Should I add some necessary changes for XLA, or does it better to skip the XLA test by adding annotation?

mruberry · 2020-07-20T08:18:55Z

Hi, @mruberry ! I implemented the torch.signbit() method and it's now ready to be reviewed. It seems that there is an XLA-related error occured in the CI test. Should I add some necessary changes for XLA, or does it better to skip the XLA test by adding annotation?

Great! Thanks @Kiyosora! I'll take a look soon.

aten/src/ATen/native/native_functions.yaml

ailzhang

stamping for xla fix :D Thanks!

mruberry

Thank you, @ailzhang, for helping with the XLA dispatch!

This is a well-written PR, @Kiyosora, thanks for submitting it!

I made a few comments about the test and docs, but also have a question about the design of this PR: does it need a new TensorIterator kernel, or can it be implemented using existing PyTorch functions? I'm curious to hear your thoughts.

aten/src/ATen/native/UnaryOps.cpp

aten/src/ATen/native/native_functions.yaml

test/test_torch.py

torch/_torch_docs.py

Kiyosora · 2020-07-28T05:26:53Z

Hi, @mruberry ! Thank you so much for your advice & Sorry for my late reply. 😢
Based on your advice, I improved my code. Please make review does it reach the goal?

mruberry · 2020-07-28T11:33:01Z

aten/src/ATen/native/UnaryOps.cpp

It's true that clamp is not implemented for complex tensors, but this should probably say:

"signbit is not implemented for complex tensors."

No need for a "yet," either, since we have no plans to implement it in the future -- it's not even clear how to define a function like signbit on complex values. NumPy, for example, also throws an error when signbit is given a complex input.

Also: better add a check that result is bool

Addressed! Thanks for the correction.

mruberry · 2020-07-28T11:44:53Z

aten/src/ATen/native/UnaryOps.cpp

This doesn't need to promote inputs or cast to an output. There's only one input and for this first iteration signbit doesn't need to support non-bool outputs.

Got it! thanks for your explanation.

mruberry · 2020-07-28T11:47:52Z

aten/src/ATen/native/native_functions.yaml

(Tensor self), no star needed here

mruberry · 2020-07-28T11:48:47Z

torch/_torch_docs.py

\operatorname{signbit} ?

The formulas has been deleted, Sorry for my carelessness.

mruberry · 2020-07-28T11:50:18Z

test/test_torch.py

This regexes will need to be updated to refer to signbit

mruberry

Overall looks very good!

I made a few comments with updates/corrections.

Kiyosora · 2020-07-29T11:52:36Z

Hi, @mruberry. I'm sorry that my carelessness caused some avoidable mistakes.
I have fixed them, Would you please help me to check them again, thanks! 🙏

mruberry

Nice work, @Kiyosora!

Let me know if you're interested in implementing another function or would like to look at a different kind of issue.

facebook-github-bot

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

Kiyosora · 2020-07-30T02:52:21Z

Nice work, @Kiyosora!

Let me know if you're interested in implementing another function or would like to look at a different kind of issue.

Thanks for confirm, @mruberry !

I'd' like going to work on the function heaviside() , another PR would be come up soon.

mruberry · 2020-07-30T05:15:30Z

Nice work, @Kiyosora!
Let me know if you're interested in implementing another function or would like to look at a different kind of issue.

Thanks for confirm, @mruberry !

I'd' like going to work on the function heaviside() , another PR would be come up soon.

Great! Looking forward to it.

facebook-github-bot · 2020-07-30T20:20:55Z

@mruberry merged this pull request in 26d5850.

pytorchbot added the open source label Jul 17, 2020

Kiyosora force-pushed the signbit_implement branch from eb278ce to bd179e2 Compare July 17, 2020 12:31

Kiyosora marked this pull request as ready for review July 20, 2020 01:28

Kiyosora force-pushed the signbit_implement branch from bd179e2 to b65627a Compare July 20, 2020 02:25

Kiyosora changed the title ~~[WIP] Implementing NumPy-like function torch.signbit()~~ Implementing NumPy-like function torch.signbit() Jul 20, 2020

mruberry self-requested a review July 20, 2020 08:18

mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Jul 20, 2020

ailzhang requested changes Jul 20, 2020

View reviewed changes

aten/src/ATen/native/native_functions.yaml Outdated Show resolved Hide resolved

gchanan added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 20, 2020

Kiyosora requested a review from ailzhang July 21, 2020 04:02

ailzhang approved these changes Jul 21, 2020

View reviewed changes

mruberry requested changes Jul 21, 2020

View reviewed changes

Kiyosora force-pushed the signbit_implement branch from 7b76fb9 to 359d5d1 Compare July 28, 2020 01:34

Kiyosora requested a review from mruberry July 28, 2020 05:27

mruberry reviewed Jul 28, 2020

View reviewed changes

mruberry self-requested a review July 28, 2020 11:53

mruberry requested changes Jul 28, 2020

View reviewed changes

Kiyosora force-pushed the signbit_implement branch 3 times, most recently from 5e95f7f to 775f095 Compare July 29, 2020 08:54

Kiyosora requested a review from mruberry July 29, 2020 11:40

kiyosora added 5 commits July 30, 2020 09:18

implement for torch.signbit()

4135e55

fix xla error

0667f7d

Improve signbit kernel

ce31ef9

fix the mistakes

65087f0

add test case of non-boolean outputs

9db39c6

Kiyosora force-pushed the signbit_implement branch from 775f095 to 9db39c6 Compare July 30, 2020 01:19

mruberry approved these changes Jul 30, 2020

View reviewed changes

facebook-github-bot reviewed Jul 30, 2020

View reviewed changes

facebook-github-bot closed this in 26d5850 Jul 30, 2020

mruberry mentioned this pull request Jul 30, 2020

NumPy-like Functionality Request Rollup #38349

Closed

41 tasks

facebook-github-bot added the merged label Jul 30, 2020

mruberry added the Merged label Oct 28, 2020

Implementing NumPy-like function torch.signbit() #41589

Implementing NumPy-like function torch.signbit() #41589

Uh oh!

Conversation

Kiyosora commented Jul 17, 2020

Uh oh!

dr-ci bot commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🚧 3 fixed upstream failures:

Uh oh!

Kiyosora commented Jul 20, 2020

Uh oh!

mruberry commented Jul 20, 2020

Uh oh!

Uh oh!

ailzhang left a comment

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kiyosora commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

Kiyosora commented Jul 29, 2020

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Kiyosora commented Jul 30, 2020

Uh oh!

mruberry commented Jul 30, 2020

Uh oh!

facebook-github-bot commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dr-ci bot commented Jul 17, 2020 •

edited

Loading