Add segsort from moderngpu #181

csukuangfj · 2020-09-26T04:21:19Z

Closes #176

danpovey · 2020-09-26T04:27:43Z

k2/csrc/moderngpu_context.cu

+
+namespace {
+
+class ModernGpuContext : public mgpu::standard_context_t {


i wonder if this should be called ModernGpuAllocator to clarify that it's not an instance of k2::Context.

danpovey

That was fast! Thanks!

csukuangfj · 2020-09-26T04:40:32Z

There exists an issue for the current commit:

k2/k2/csrc/ragged_inl.h

Lines 114 to 129 in 5a9fc1c

    
           mgpu::segmented_sort_indices(src->values.Data(),  // keys 
        
                                        order->Data(),       // indices 
        
                                        src->values.Dim(),   // count 
        
                                        segment.Data() + 1,  // segments 
        
                                        segment.Dim() - 1,   // num_segments 
        
                                        Op(),                // cmp 
        
                                        *context);           // context 
        
           auto err = cudaGetLastError(); 
        
           (void)err; 
        
           // TODO(fangjun): err is not cudaSuccess, but why was the data sorted 
        
           // correctly? 
        
           // 
        
           // Check failed: err == cudaSuccess (9 vs. 0)  Error: invalid configuration 
        
           // argument. 
        
           // 
        
           // K2_DCHECK_CUDA_ERROR(err);

The cuda error code for mgpu::segmented_sort_indices is not cudaSuccess, but it sorts the data correctly.

danpovey · 2020-09-26T05:59:26Z

Thanks!!
At some point it should be possible to implement ArcSort (see fsa_algo.h).

qindazhu · 2020-09-26T11:12:34Z

k2/csrc/ragged_inl.h

+  std::unique_ptr<mgpu::context_t> context =
+      GetModernGpuAllocator(src->Context()->GetDeviceId());
+
+  Array1<int32_t> &segment = src->shape.RowSplits(src->NumAxes() - 1);


it's better to add K2_CHECK_EQ(src->shape.NumAxes() == 2 at entry as we just sort values in src

It supports ragged arrays with more than two axes.

Well, at least check >=2 here otherwise there will be an error when calling shape.RowSplits(NumAxes() - 1), note that you should add such requirements in the documentation

k2/k2/csrc/ragged.h

Lines 577 to 584 in a178524

Sort a ragged array in-place.

@param [inout] The input array to be sorted.

CAUTION: it is sorted in-place.

@param [out] The indexes mapping from the sorted

array to the input array. The caller

has to pre-allocate memory for it

on the same device as `src`.

BTW, in other APIs, such as MaxPerSublist, we require NumAxes() ==2 now. @danpovey I think we should make those api consistent, should we require exactly NumAxes() == 2 or NumAxes() >=2

In general I'd like to make the APIs as general as they can be made without requiring extra work.

It has no requirement about the number of axes a ragged array has. It always sorts the last axis.
This function works as long as the ragged array is not empty.

mm.. definitely it requires NumAxes >=2...It's meaningless to call RowSplits(1) on a shape with NumAxes < 2, actually it will crash..

Then I suggest we add documentation and requirements for those APIs with axes >= 2

Could you give an example to show what a ragged array with NumAxes < 2 looks like?

mm, the point here is, now we allow user define a empty RaggedShape technically,

k2/k2/csrc/ragged.h

Line 137 in a178524

RaggedShape() = default;

, so it's better if we can check this and report error as early as possible (i.e. at entry of the function).

I'll add a K2_DCHECK in the next commit.

it's better to add K2_CHECK_EQ(src->shape.NumAxes() == 2 at entry as we just sort values in src

@qindazhu
fixed in #218

k2/k2/csrc/ragged_inl.h

Line 111 in d291daf

K2_DCHECK_GE(src->NumAxes(), 2);

qindazhu · 2020-09-26T11:13:18Z

k2/csrc/ragged_inl.h

+  mgpu::segmented_sort_indices(src->values.Data(),  // keys
+                               order->Data(),       // indices
+                               src->values.Dim(),   // count
+                               segment.Data() + 1,  // segments


why +1 here? the api of mgpu is strange,

I also tried segment.Data() and got the same result.

According to https://github.com/moderngpu/moderngpu/blob/2b3985541c8e88a133769598c406c33ddde9d0a5/tests/test_segsort.cu#L9-L15

std::vector<int> cpu_segsort(const std::vector<int>& data, const std::vector<int>& segments) { std::vector<int> copy = data; int cur = 0; for(int seg = 0; seg < segments.size(); ++seg) { int next = segments[seg]; std::sort(copy.data() + cur, copy.data() + next); cur = next; } std::sort(copy.data() + cur, copy.data() + data.size()); return copy; }

The start index of the first segment is 0. That's why I use segment.Data() + 1.

danpovey · 2020-09-27T05:41:36Z

Checking is fine. I dont think we need to document it because any valid RaggedShape automatically has NumAxes() > 2. But of course clarify what the function does if it's not already clear.

…

On Sun, Sep 27, 2020 at 1:27 PM Haowen Qiu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In k2/csrc/ragged_inl.h <#181 (comment)>: > @@ -95,6 +100,35 @@ Ragged<T> RandomRagged(T min_value, T max_value, int32_t min_num_axes, return Ragged<T>(shape, values); } +template <typename T, typename Op /* = LessThan<T> */> +void SortSublists(Ragged<T> *src, Array1<int32_t> *order) { + K2_DCHECK(IsCompatible(src->values, *order)); + K2_DCHECK_EQ(src->values.Dim(), order->Dim()); + K2_DCHECK_EQ(src->Context()->GetDeviceType(), kCuda) + << "It supports only CUDA at present"; + + std::unique_ptr<mgpu::context_t> context = + GetModernGpuAllocator(src->Context()->GetDeviceId()); + + Array1<int32_t> &segment = src->shape.RowSplits(src->NumAxes() - 1); mm, the point here is, now we allow user define a empty RaggedShape technically, https://github.com/k2-fsa/k2/blob/a17852434c87f76e2dfc837918197c98b7cb947d/k2/csrc/ragged.h#L137, so it's better if we can check this and report error as early as possible (i.e. at entry of the function). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZFLO56M3UEIBCUNEQEJSTSH3EM7ANCNFSM4R2U4NOQ> .

csukuangfj · 2020-10-04T05:55:38Z

At some point it should be possible to implement ArcSort (see fsa_algo.h).

@danpovey

Are arcs sorted by ilabel or by olabel?

danpovey · 2020-10-04T06:07:07Z

There is only one label in the arc.

…

On Sun, Oct 4, 2020 at 1:55 PM Fangjun Kuang ***@***.***> wrote: At some point it should be possible to implement ArcSort (see fsa_algo.h). @danpovey <https://github.com/danpovey> Are arcs sorted by ilabel or by olabel? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZFLO7RRV7ASD7HTDOHHYTSJAE6LANCNFSM4R2U4NOQ> .

danpovey · 2020-10-04T06:07:42Z

BTW I have declared ArcSort in fsa_algo.h.

…

On Sun, Oct 4, 2020 at 2:06 PM Daniel Povey ***@***.***> wrote: There is only one label in the arc. On Sun, Oct 4, 2020 at 1:55 PM Fangjun Kuang ***@***.***> wrote: > At some point it should be possible to implement ArcSort (see fsa_algo.h). > > @danpovey <https://github.com/danpovey> > > Are arcs sorted by ilabel or by olabel? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#181 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAZFLO7RRV7ASD7HTDOHHYTSJAE6LANCNFSM4R2U4NOQ> > . >

danpovey · 2020-10-04T08:41:25Z

Guys, IMO it would have been more convenient to allow SortSublists to accept arrays with any num-axes but operate on the last one. So it would access the last row-ids or row-splits. Not urgent though.

…

On Sun, Oct 4, 2020 at 3:58 PM Fangjun Kuang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In k2/csrc/ragged_inl.h <#181 (comment)>: > @@ -95,6 +100,35 @@ Ragged<T> RandomRagged(T min_value, T max_value, int32_t min_num_axes, return Ragged<T>(shape, values); } +template <typename T, typename Op /* = LessThan<T> */> +void SortSublists(Ragged<T> *src, Array1<int32_t> *order) { + K2_DCHECK(IsCompatible(src->values, *order)); + K2_DCHECK_EQ(src->values.Dim(), order->Dim()); + K2_DCHECK_EQ(src->Context()->GetDeviceType(), kCuda) + << "It supports only CUDA at present"; + + std::unique_ptr<mgpu::context_t> context = + GetModernGpuAllocator(src->Context()->GetDeviceId()); + + Array1<int32_t> &segment = src->shape.RowSplits(src->NumAxes() - 1); it's better to add K2_CHECK_EQ(src->shape.NumAxes() == 2 at entry as we just sort values in src @qindazhu <https://github.com/qindazhu> fixed in #218 <#218> https://github.com/k2-fsa/k2/blob/d291daf7a88d50907d2b1039ca6033f307995a65/k2/csrc/ragged_inl.h#L111 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#181 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZFLO7OXHPBYV3XLYRGGP3SJATI3ANCNFSM4R2U4NOQ> .

danpovey · 2020-10-04T08:41:51Z

... for instance, the way it is now would require adding extra code in the implementation of ArcSort (else it wouldn't work for FsaVec).

…

On Sun, Oct 4, 2020 at 4:41 PM Daniel Povey ***@***.***> wrote: Guys, IMO it would have been more convenient to allow SortSublists to accept arrays with any num-axes but operate on the last one. So it would access the last row-ids or row-splits. Not urgent though. On Sun, Oct 4, 2020 at 3:58 PM Fangjun Kuang ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In k2/csrc/ragged_inl.h > <#181 (comment)>: > > > @@ -95,6 +100,35 @@ Ragged<T> RandomRagged(T min_value, T max_value, int32_t min_num_axes, > return Ragged<T>(shape, values); > } > > +template <typename T, typename Op /* = LessThan<T> */> > +void SortSublists(Ragged<T> *src, Array1<int32_t> *order) { > + K2_DCHECK(IsCompatible(src->values, *order)); > + K2_DCHECK_EQ(src->values.Dim(), order->Dim()); > + K2_DCHECK_EQ(src->Context()->GetDeviceType(), kCuda) > + << "It supports only CUDA at present"; > + > + std::unique_ptr<mgpu::context_t> context = > + GetModernGpuAllocator(src->Context()->GetDeviceId()); > + > + Array1<int32_t> &segment = src->shape.RowSplits(src->NumAxes() - 1); > > it's better to add K2_CHECK_EQ(src->shape.NumAxes() == 2 at entry as we > just sort values in src > > @qindazhu <https://github.com/qindazhu> > fixed in #218 <#218> > > > https://github.com/k2-fsa/k2/blob/d291daf7a88d50907d2b1039ca6033f307995a65/k2/csrc/ragged_inl.h#L111 > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#181 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAZFLO7OXHPBYV3XLYRGGP3SJATI3ANCNFSM4R2U4NOQ> > . >

csukuangfj · 2020-10-04T12:00:10Z

Guys, IMO it would have been more convenient to allow SortSublists to
accept arrays with any num-axes but operate on the last one.

This is what the current implementation does, except that it requires a NON-empty input.

csukuangfj added 11 commits September 25, 2020 23:56

[WIP] add moderngpu.

e0d17a0

add a toy example.

a2f82b9

add segsort.

533406e

test segsort with a random Ragged array.

9cb83a5

add unittest for segsort.

c09a3ed

add more tests for segsort.

a8120f5

replace moderngpu's standard_context_t.

ce9eddd

remove cpu support from ModernGpuContext.

01d1091

fix typos.

89c8595

remove extra files.

8f2f8e0

fix style issues.

6c14c1c

csukuangfj requested review from danpovey and qindazhu September 26, 2020 04:27

danpovey reviewed Sep 26, 2020

View reviewed changes

rename ModernGpuContext to ModernGpuAllocator.

5a9fc1c

fix the return type of LessThan.

0eaf4df

danpovey merged commit ee562fc into k2-fsa:master Sep 26, 2020

qindazhu reviewed Sep 26, 2020

View reviewed changes

csukuangfj deleted the fangjun-moderngpu branch September 29, 2020 13:52

csukuangfj mentioned this pull request Nov 7, 2021

Fix compilation errors with CUB 1.15. #865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add segsort from moderngpu #181

Add segsort from moderngpu #181

csukuangfj commented Sep 26, 2020

danpovey Sep 26, 2020

csukuangfj Sep 26, 2020

danpovey left a comment

csukuangfj commented Sep 26, 2020

danpovey commented Sep 26, 2020

qindazhu Sep 26, 2020

csukuangfj Sep 26, 2020

qindazhu Sep 27, 2020

danpovey Sep 27, 2020

csukuangfj Sep 27, 2020

qindazhu Sep 27, 2020

csukuangfj Sep 27, 2020

qindazhu Sep 27, 2020

csukuangfj Sep 27, 2020

csukuangfj Oct 4, 2020

qindazhu Sep 26, 2020

csukuangfj Sep 26, 2020

danpovey commented Sep 27, 2020 via email

csukuangfj commented Oct 4, 2020

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

csukuangfj commented Oct 4, 2020


		namespace {

		class ModernGpuContext : public mgpu::standard_context_t {

	Sort a ragged array in-place.

	@param [inout] The input array to be sorted.
	CAUTION: it is sorted in-place.
	@param [out] The indexes mapping from the sorted
	array to the input array. The caller
	has to pre-allocate memory for it
	on the same device as `src`.

Add segsort from moderngpu #181

Add segsort from moderngpu #181

Conversation

csukuangfj commented Sep 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey left a comment

Choose a reason for hiding this comment

csukuangfj commented Sep 26, 2020

danpovey commented Sep 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpovey commented Sep 27, 2020 via email

csukuangfj commented Oct 4, 2020

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

danpovey commented Oct 4, 2020 via email

csukuangfj commented Oct 4, 2020