Skip to content
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

Add torch.nansum #38628

Closed
wants to merge 47 commits into from
Closed

Add torch.nansum #38628

wants to merge 47 commits into from

Conversation

kshitij12345
Copy link
Collaborator

Reference: #38349

@dr-ci
Copy link

dr-ci bot commented May 17, 2020 •
edited

💊 CI failures summary and remediations

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

  • 2/4 failures possibly* introduced in this PR
    • 2/2 non-CircleCI failure(s)
  • 2/4 broken upstream at merge base b6810c1 since Aug 08

🚧 2 ongoing upstream failures:

These were probably caused by upstream breakages that are not fixed yet:

ci.pytorch.org: 2 failed

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

@kshitij12345
replace std::isnan with at::_isnan
9ad7089
@kshitij12345
dispatch only to floating types CUDA
23afd15
@kshitij12345
update _overrides.py
bc905f3
@kshitij12345
changes
292325f 
* add type promotion logic mimicking numpy.
* add gradient function.
* add tests.
* update NanSumOps.
@mruberry mruberry self-requested a review May 19, 2020 08:01
@mruberry mruberry added module: numpy Related to numpy support, and also numpy compatibility of our operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels May 19, 2020
@mruberry
Copy link
Collaborator

Hey @kshitij12345! Let me know when you're ready for a review.

@kshitij12345
Copy link
Collaborator Author

@mruberry Sure. Sorry forgot to tag this as [WIP]. Will ping you once ready or if I have any doubts.

@kshitij12345 kshitij12345 changed the title Add torch.nansum [WIP] Add torch.nansum May 19, 2020
@kshitij12345 kshitij12345 changed the title [WIP] Add torch.nansum Add torch.nansum Jun 14, 2020
@kshitij12345
Copy link
Collaborator Author

kshitij12345 commented Jun 14, 2020 •
edited

@mruberry

It is ready for review now. Please review :)

@kshitij12345
Copy link
Collaborator Author

@mruberry

Have addressed the comments. Please review :)

Thanks!

@kshitij12345
Copy link
Collaborator Author

@mruberry Gentle ping:)

1 similar comment
@kshitij12345
Copy link
Collaborator Author

@mruberry Gentle ping:)

@mruberry
Copy link
Collaborator

@mruberry Gentle ping:)

Was just about to update this! It's going to take me a few days to get to because I have to go through the sum/prod changes extremely carefully.

@kshitij12345
Copy link
Collaborator Author

Sure. Thanks! Actually I am a bit sceptical related to float16 cases given that we had to increase the tolerance a lot for it.

Also would it be okay if I ping you on slack related to a doubt on another PR?

@mruberry
Copy link
Collaborator

Sure. Thanks! Actually I am a bit sceptical related to float16 cases given that we had to increase the tolerance a lot for it.

Also would it be okay if I ping you on slack related to a doubt on another PR?

Yes of course.

@mruberry
Copy link
Collaborator

This is still on my radar and I should get to it very soon.

mruberry
mruberry reviewed Jul 30, 2020
View reviewed changes
aten/src/ATen/native/cuda/ReduceSumProdKernel.cu
typename out_t = scalar_t>
typename OpFunctor,
typename GeneralDispatcher>
static void reduce_dispatch(TensorIterator& iter, GeneralDispatcher op) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code here looks elegant, but is there a performance impact on the existing functions, like sum, by building the callable structs each time this function is called?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Follow-up question from look the the templates: the functions of interest all have the same signature, right?

Could this be written, for example, as one function for each op that called a helper that handled the common part (lines 54-67 below) and then implemented the function-specific dispatch? Further, can the helper avoid be a function template by using the common signature of these functions to specify its function pointer argument?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it should not be an issue as the structs method will get inlined and construction of that struct is trivial.
Reference: https://stackoverflow.com/a/18753022/5602957

Also tried simulating a similar code on CompilerExplorer
https://godbolt.org/z/7WTWhe

Compiler is able to actually deduce the finally value, so it don't think this structure should hinder the compiler optimizations.

Follow-up question from look the the templates: the functions of interest all have the same signature, right?

Could this be written, for example, as one function for each op that called a helper that handled the common part (lines 54-67 below) and then implemented the function-specific dispatch? Further, can the helper avoid be a function template by using the common signature of these functions to specify its function pointer argument?

Slightly confused. Could you please give sample code.

Thanks for looking into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should not be an issue as the structs method will get inlined and construction of that struct is trivial.
Reference: https://stackoverflow.com/a/18753022/5602957

Also tried simulating a similar code on CompilerExplorer
https://godbolt.org/z/7WTWhe

Compiler is able to actually deduce the finally value, so it don't think this structure should hinder the compiler optimizations.

Follow-up question from look the the templates: the functions of interest all have the same signature, right?
Could this be written, for example, as one function for each op that called a helper that handled the common part (lines 54-67 below) and then implemented the function-specific dispatch? Further, can the helper avoid be a function template by using the common signature of these functions to specify its function pointer argument?

Slightly confused. Could you please give sample code.

Thanks for looking into it.

Thanks for investigating. I think that addresses my concern so this should be fine.

mruberry
mruberry reviewed Jul 30, 2020
View reviewed changes
test/test_torch.py Outdated Show resolved Hide resolved
@mruberry
Copy link
Collaborator

Hey @kshitij12345, thank you for being so patient. I took a close look and overall things look very good. I have a question about the organization of the common function to call prod, sum, and nansum. The code is very elegant, but I'm a little concerned about its effect on performance and wonder if its template logic can be further simplified. I look forward to hear your thoughts!

I should be much more responsive now, so we'll get this in quickly!

@kshitij12345
address comments
3dedc2e 
* add with_extremal for general case.
* minor refactor of test code.
@kshitij12345
Merge branch 'master' into develop/numpy/nansum
e5445d2
@mruberry
Copy link
Collaborator

Test looks great, thanks @kshitij12345!

@mruberry mruberry self-requested a review July 31, 2020 03:27
mruberry
mruberry approved these changes Jul 31, 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.

Nice work, @kshitij12345! This is the first "nan*" function in PyTorch!

facebook-github-bot
facebook-github-bot reviewed Jul 31, 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.

@mruberry
Copy link
Collaborator

mruberry commented Aug 4, 2020

Update: this triggered some internal perf warnings. Rerunning some tests now to verify.

@mruberry
Copy link
Collaborator

mruberry commented Aug 7, 2020

Tests came back negative again. Going to try one more time. We may need to refactor this.

@kshitij12345
Copy link
Collaborator Author

Thanks for the heads-up. Let me know if there are any changes needed :).

facebook-github-bot
facebook-github-bot reviewed Aug 11, 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.

@mruberry
Copy link
Collaborator

Follow-up perf runs suggest the initial failure was flakiness in the benchmark. Initiating land process.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in ab0a04d.

@mruberry mruberry mentioned this pull request Aug 12, 2020
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albanD albanD albanD left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@mruberry mruberry mruberry approved these changes

Assignees
No one assigned
Labels
Merged module: numpy Related to numpy support, and also numpy compatibility of our operators open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kshitij12345 @mruberry @facebook-github-bot @albanD @pytorchbot