Implement kthvalue in ATen #17544
Closed
Implement kthvalue in ATen #17544
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The CPU version is based on the TH version. The GPU version is based on pytorch#8406 by Pararth Shah (thank you). As discussed with @izdeby, this splits out a small(ish) chunk from pytorch#17232, the other bits will follow in spearate PRs. Plan for PRs: - This - Sort - TopK + Mode + Median in any order - Rip out THC stuff. There may be utility functions in the SortingCommon.cuh that come into relevance only with sort. I hope this is more digestible..
izdeby
suggested changes
Feb 28, 2019
| *topK = TopKTypeConfig<scalar_t>::deconvert(desired); | ||
| } | ||
| } // namespace native | ||
| } // namespace at |
There was a problem hiding this comment.
two questions about this file:
- Im just curious why is gatherTopK in cuda/SortingKthValue.cu instead of being here?
- Did you check if some the ported code can me deleted in TH or is it still being used somewhere?
There was a problem hiding this comment.
- I only moved those bits into the cuh that are used by more than one file eventually, but I can move it to the .cuh if you prefer. (But I hope it's gatherKthValue. :) ) That said I'm not entirely sure whether it's still as relevant as it used to be, but the THC code went to great length to reduce the sizes of compilation units - splitting the scalar type files etc., so I think keeping the .cuh minimal is a feature.
- I don't think much can go - the THC code needs to stay in until we move topk. Also mode needs kth value for now.
|
Can we kill the _th_kthvalue declaration in the Declarations.cwrap as a part of this PR or is it still needed? |
izdeby
approved these changes
Feb 28, 2019
|
LGTM. lets make sure that all tests are passing and its good to go |
|
@pytorchbot merge this, please |
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
Mar 2, 2019
Summary: The CPU version is based on the TH version. The GPU version is based on #8406 by Pararth Shah (thank you). CPU quickselect based on that in TH's THTensorMoreMath.cpp, but with C++ (quickselectnoindex will be achieved by a different swap) CPU kthvalue is based on the THTensor function in the same file. The dim_apply function is a C++ replacement for TH_TENSOR_DIM_APPLYx macros. The CUDA kernel uses functions adapted from the THCTensorSortK implementation. In particular radixSelect is from THCTensorTopK.cuh. The CUDA launcher code replaces a bunch of macros with C++. It will be re-used in one of the following patches. Plan for further PRs: - This - Sort - TopK + Mode + Median in any order - Rip out THC stuff. There may be utility functions / structs in the SortingCommon.cuh that come into relevance only with sort. Pull Request resolved: pytorch/pytorch#17544 Differential Revision: D14286934 Pulled By: ezyang fbshipit-source-id: 35dbea050b097e88777ac5fa5c0f499d5e23c738
Closed
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 21, 2019
Summary: This moves median to ATen. - median with dimension reduces to kthvalue - median without dimension (aka medianall) is implemented in parallel to kthvalue because we would not want to reshape (copying for non-contiguous) and then copy again in kthvalue. We can sue the helper functions we moved from kthvalue. - `median_cuda` was accidentally already put into ATen in #17544. - The quickselect algorthm without indices for CPU in TH is now obsolete and removed. Pull Request resolved: #17637 Differential Revision: D14346510 Pulled By: ezyang fbshipit-source-id: c07ad144efbd6b4194179bb1c02635862521d8cb
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The CPU version is based on the TH version.
The GPU version is based on #8406 by Pararth Shah (thank you).
CPU quickselect based on that in TH's THTensorMoreMath.cpp, but with C++ (quickselectnoindex will be achieved by a different swap)
CPU kthvalue is based on the THTensor function in the same file.
The dim_apply function is a C++ replacement for TH_TENSOR_DIM_APPLYx macros.
The CUDA kernel uses functions adapted from the THCTensorSortK implementation.
In particular radixSelect is from THCTensorTopK.cuh.
The CUDA launcher code replaces a bunch of macros with C++. It will be re-used in one of the following patches.
Plan for further PRs:
There may be utility functions / structs in the SortingCommon.cuh that come into
relevance only with sort.