Skip to content

Add torch.count_nonzero #39992

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
kshitij12345 wants to merge 19 commits into from
Closed

Add torch.count_nonzero #39992

kshitij12345 wants to merge 19 commits into from

Conversation

@kshitij12345
Copy link
Collaborator

@kshitij12345 kshitij12345 commented Jun 13, 2020 •
edited
Loading

Reference #38349

TODO:

  • Add tests
  • Add docs (pending add to docs.rst)

@kshitij12345 kshitij12345 changed the title Add torch.count_nonzero [WIP] Add torch.count_nonzero Jun 13, 2020
@kshitij12345 kshitij12345 marked this pull request as draft June 13, 2020 15:22
@dr-ci
Copy link

dr-ci bot commented Jun 13, 2020 •
edited
Loading

💊 CI failures summary and remediations

As of commit 61a9937 (more details on the Dr. CI page):

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

🚧 1 fixed upstream failure:

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.

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 46 times.

@kshitij12345 kshitij12345 marked this pull request as ready for review June 14, 2020 15:07
@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 14, 2020

@mruberry

Please review

@kshitij12345 kshitij12345 changed the title [WIP] Add torch.count_nonzero Add torch.count_nonzero Jun 14, 2020
@mruberry mruberry self-requested a review June 15, 2020 07:35
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry requested changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Hey @kshitij12345, thanks for the PR! I added some comments.

Summary + followup checklist:

  • Suggestion to simplify test in test_torch.py
  • Minor doc improvements
  • Method variants
  • Add a "not_implemented" entry to tools/autograd/derivatives.yaml
  • Update docs/source/name_inference.rst
  • Update docs/source/tensors.rst and docs/source/torch.rst
  • Update torch/_torch_docs.py

Overall this is really good, it just needs a few updates.

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 15, 2020

@mruberry
Thanks for the great review with summary!

I believe have addressed the comments. Please review.

mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry reviewed Jun 15, 2020
View reviewed changes
mruberry
mruberry requested changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

A few more test changes (sorry about that, I made a note to make self.compare_with_numpy easier to use!) and I suggest you remove the name variant to keep this PR more focused, otherwise you'll need to add tests for it, and that could be a separate PR if you wanted.

Also I made a note on the name_inference.rst doc.

@kshitij12345
remove named tensor support
dd13edd
@kshitij12345
address comments
f233264 
* scale the randomly generated values to have bigger range for intergral.
* pass the input data as tolist(), this will force compare_with_numpy,
  to obey the passed device and dtype.
@mruberry
Copy link
Collaborator

mruberry commented Jun 16, 2020

#40064 clarifies the compare_with_numpy behavior, thank you for revealing an issue with it. You will have to rebase after it goes in, unfortunately, but the code change it requires is very small. We really want to avoid those tolist() calls.

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 16, 2020

@mruberry

PTAL:)

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 18, 2020

@mruberry

Would be great if you can have another look at this. I am planning to use the _generate_input and _test_reduction_function_with_numpy for few tests for nansum.

Thank You.

mruberry
mruberry reviewed Jun 18, 2020
View reviewed changes
mruberry
mruberry approved these changes Jun 18, 2020
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Excellent work. This looks really good, @kshitij12345!

Should the test also consider complex dtypes?

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 22, 2020

@mruberry

Have added complex dtypes to the test as well.
I guess that should cover all the bases.

@mruberry
Copy link
Collaborator

mruberry commented Jun 24, 2020

@mruberry

Have added complex dtypes to the test as well.
I guess that should cover all the bases.

Thanks kshitij12345! We just need to wait for the tests again since there was a merge conflict with master.

@mruberry
Copy link
Collaborator

mruberry commented Jun 24, 2020

Another merge conflict, unfortunately. We'll have to wait for the tests again.

facebook-github-bot
facebook-github-bot reviewed Jun 25, 2020
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.

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

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 27, 2020

@mruberry Gentle ping :)

@mruberry
Copy link
Collaborator

mruberry commented Jun 27, 2020

@mruberry Gentle ping :)

Thanks for the ping. I'm fighting with our internal landing system, unfortunately.

@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 28, 2020

Oh. Sure. Thank You! :)

@mruberry mruberry mentioned this pull request Jun 30, 2020
Closed
2 tasks
@mruberry mruberry mentioned this pull request Jun 30, 2020
Closed
41 tasks
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

@mruberry mruberry mruberry approved these changes

Assignees

No one assigned

Labels

open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kshitij12345 @mruberry @facebook-github-bot @pytorchbot