Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Port adjacent difference into CUB #331

Conversation

gevtushenko
Copy link
Collaborator

The PR contains:

  1. a port of thrust::adjacent_difference algorithm;
  2. deprecation of FlagHeads and FlagTails methods from BlockAdjacentDifference structure;
  3. fixed API for BlockAdjacentDifference along with documentation and tests.

Note that the PR is based on some of the features introduced in MergeSort porting.

@gevtushenko gevtushenko force-pushed the main-feature/github/cub_adjacent_difference branch from 10db20a to 891c10c Compare June 29, 2021 15:26
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

This is a great first pass 👍 Let me know when you're ready for a re-review.

cub/agent/agent_merge_sort.cuh Outdated Show resolved Hide resolved
cub/block/block_adjacent_difference.cuh Outdated Show resolved Hide resolved
cub/block/block_adjacent_difference.cuh Outdated Show resolved Hide resolved
cub/block/block_adjacent_difference.cuh Show resolved Hide resolved
cub/block/block_adjacent_difference.cuh Outdated Show resolved Hide resolved
test/test_block_adjacent_difference.cu Outdated Show resolved Hide resolved
cub/block/block_adjacent_difference.cuh Show resolved Hide resolved
test/test_block_adjacent_difference.cu Outdated Show resolved Hide resolved
test/test_block_adjacent_difference.cu Outdated Show resolved Hide resolved
test/test_block_adjacent_difference.cu Outdated Show resolved Hide resolved
@alliepiper alliepiper added this to the 1.14.0 milestone Jul 22, 2021
@alliepiper alliepiper assigned gevtushenko and unassigned alliepiper Jul 22, 2021
@alliepiper alliepiper modified the milestones: 1.14.0, 1.15.0 Aug 17, 2021
@alliepiper alliepiper added the P1: should have Necessary, but not critical. label Aug 17, 2021
@gevtushenko gevtushenko force-pushed the main-feature/github/cub_adjacent_difference branch 2 times, most recently from 3c13360 to 53ffc37 Compare August 23, 2021 09:27
@gevtushenko gevtushenko force-pushed the main-feature/github/cub_adjacent_difference branch 2 times, most recently from 370bee7 to 88a4ae8 Compare August 27, 2021 15:27
@alliepiper alliepiper assigned alliepiper and unassigned gevtushenko Sep 21, 2021
@alliepiper alliepiper added type: enhancement New feature or request. P2: nice to have Desired, but not necessary. and removed P1: should have Necessary, but not critical. labels Oct 14, 2021
@alliepiper alliepiper modified the milestones: 1.15.0, 1.16.0 Oct 14, 2021
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

LGTM -- just some comments on documentation, otherwise this is ready to start testing 👍

* `{ ...3], [4,2,1,1], [1,1,1,1], [2,3,3,3], [3,4,1,4] }`.
* and that `valid_items` is `507`. The corresponding output `result` in
* those threads will be
* `{ ..., [-1,2,1,0], [0,0,0,-1], [-1,0,3,3], [3,4,1,4] }`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(continuing from #331 (comment))

Gotcha, so the last 5 values are unchanged random values that are ignored by the algorithm.

There's another convention that uses - to stand in for such values, e.g.

* float* d_samples; // e.g., [2.2, 6.1, 7.1, 2.9, 3.5, -, -,
* // 0.3, 2.9, 2.1, 6.1, 999.5, -, -]

I think it's a little easier to parse with the non-numeric characters, since it's immediately clear that they can't be involved in the calculation. If you agree, let's change this to

* `{ ...3], [4,2,1,1], [1,1,1,1], [2,3,3,-], [-,-,-,-] }`.
* `{ ..., [-1,2,1,0], [0,0,0,-1], [-1,0,3,-], [-,-,-,-] }`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha, so the last 5 values are unchanged random values that are ignored by the algorithm.

These values aren't ignored by the algorithm:

As the documentation states, the input value is copied without modifications if the neighbour value is out of bounds.

If input and output parameters point to different memory locations, these values will be copied without modification. I'm afraid that non-numeric characters would mean that the output values are unchanged at all, for example:

input { 1, 2, 3 };
output { 4, 5, 6 };
valid_items = 0;

adjacent_difference(input, output, valid_items);

// output should be { 1, 2, 3 } and not { 4, 5, 6 }

The same with std::adjacent_difference documentation. The output for:

std::vector v {2, 4, 6, 8, 10, 12, 14, 16, 18, 20};

is

2 2 2 2 2 2 2 2 2 2

rather than:

- 2 2 2 2 2 2 2 2 2

Do you think it'll be clear from the - notation that the values are copied?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a good point -- I'm fine leaving this as-is.

* __global__ void ExampleKernel(...)
* {
* // Specialize BlockAdjacentDifference for a 1D block of
* // 128 threads on type int
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/on/of/

(this is repeated in a few other docstrings)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are quite a few places with this typo:

rg "threads on type" | wc -l
33

I'll fix this in the block_scan, block_discontinuity and block_reduce documentation as well.

@alliepiper alliepiper assigned gevtushenko and unassigned alliepiper Nov 30, 2021
@gevtushenko gevtushenko force-pushed the main-feature/github/cub_adjacent_difference branch from 28c79e4 to a64d24a Compare November 30, 2021 21:05
@gevtushenko gevtushenko force-pushed the main-feature/github/cub_adjacent_difference branch from f83d279 to 6d052db Compare December 11, 2021 11:13
@gevtushenko gevtushenko added testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Dec 11, 2021
@gevtushenko
Copy link
Collaborator Author

gpuCI: NVIDIA/thrust/pull/1577
DVS: 30766806

@gevtushenko gevtushenko added testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS). release: breaking change Include in "Breaking Changes" section of release notes. and removed testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Dec 12, 2021
@gevtushenko gevtushenko merged commit 722e3ca into NVIDIA:main Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2: nice to have Desired, but not necessary. release: breaking change Include in "Breaking Changes" section of release notes. testing: gpuCI passed Passed gpuCI testing. testing: internal ci passed Passed internal NVIDIA CI (DVS). type: enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants