Skip to content

Have median reduce over all dims and return just the value if dim is not provided#1741

Closed
lantiga wants to merge 4 commits intopytorch:masterfrom
lantiga:median_fix
Closed

Have median reduce over all dims and return just the value if dim is not provided#1741
lantiga wants to merge 4 commits intopytorch:masterfrom
lantiga:median_fix

Conversation

@lantiga
Copy link
Contributor

@lantiga lantiga commented Jun 7, 2017

Solves #1692.

@lantiga lantiga changed the title Have median reduce over all dims and return just the value dim is not provided Have median reduce over all dims and return just the value if dim is not provided Jun 7, 2017
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

The changes look good but are a bit incomplete. To properly support distributed master-worker you also need to add medianall as part of the Function enum, register the handler in master_worker/worker/Dispatch.cpp and add an RPC stub in master_worker/master/generic/THDTensor.* (both in header and the C file).

@lantiga
Copy link
Contributor Author

lantiga commented Jun 10, 2017

Thank you @apaszke, admittedly I didn't delve deep enough in distributed. Your pointers are a great start, I'll complete the PR asap.

Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Looks good now! Do you plan to add a THC implementation? For now it can just do:

  1. view as a flat vector
  2. sort
  3. memcpy the scalar

@lantiga
Copy link
Contributor Author

lantiga commented Jun 10, 2017

Sure thing

@lantiga
Copy link
Contributor Author

lantiga commented Jun 17, 2017

Sorry @apaszke , I could only get to this today.

I'm using thrust::stable_sort, but operator< is not defined for f16. I tried to introduce it with

__host__ __device__ inline bool operator<(const half &lhs, const half &rhs) 
{
   return __hlt(lhs, rhs);
}

but compilation fails complaining that __hlt is not defined (while it's documented). I can't find which is the missing include.
I can eventually use a CUDA_HALF_TENSOR guard, but do you have any hints for making the above work? Thanks in advance

@lantiga
Copy link
Contributor Author

lantiga commented Jun 17, 2017

Probably the answer is: use THCTensor_(sort) after viewing it as a 1D tensor. I just wanted to avoid generating indices, as sort currently does. We just need the value for medianall.

@apaszke
Copy link
Contributor

apaszke commented Jun 17, 2017

You can use THCNumerics

@lantiga
Copy link
Contributor Author

lantiga commented Jun 17, 2017

Doh!

@lantiga
Copy link
Contributor Author

lantiga commented Jun 21, 2017

With the last commit I added both median and medianall to THC. In the end I used _(sort) for both.

@apaszke where should I move the two functions in terms of location?
Right now they're in THCTensorMathReduce just because they're next to min and max, but that doesn't make a lot of sense. I'd say either THCTensorMath or THCTensorMathCompare?

@soumith
Copy link
Collaborator

soumith commented Jun 22, 2017

after this is rebased, this is good to go.

@lantiga
Copy link
Contributor Author

lantiga commented Jun 26, 2017

@soumith rebased

@soumith
Copy link
Collaborator

soumith commented Jul 4, 2017

this is now merged into master. Thanks a lot Luca!

@soumith soumith closed this Jul 4, 2017
jjsjann123 added a commit to jjsjann123/pytorch that referenced this pull request Jun 7, 2022
Fixes pytorch#1741

Relaxing vectorization check to avoid false assert on strides for broadcasted domains
Shamelessly stole Kevin's cpp repro.
jagadish-amd pushed a commit to jagadish-amd/pytorch that referenced this pull request Jan 14, 2025
…rch#1741)

amax was removed from _scaled_mm by pytorch#128683. Remove it from the internal
at::cuda::blas::scaled_gemm, as well. This allows hipBLASLt to find
additional solutions rather than forcing amax to be used and then
discarding the result. Pull Request resolved:
pytorch#135421 Approved by:
https://github.com/drisspg, https://github.com/eqy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants