Skip to content

keepdims should always preserve all dimensions in CUB-based reductions#2725

Merged
emcastillo merged 13 commits intocupy:masterfrom
grlee77:cub_keepdims_fix
Dec 23, 2019
Merged

keepdims should always preserve all dimensions in CUB-based reductions#2725
emcastillo merged 13 commits intocupy:masterfrom
grlee77:cub_keepdims_fix

Conversation

@grlee77
Copy link
Copy Markdown
Member

@grlee77 grlee77 commented Nov 27, 2019

closes #2720

The bug is fixed by the lines computing out_shape within device_reduce.

In order to avoid multiple calls to _get_axis, I moved the _get_axis call outside of _preprocess_array and call it directly in _routines_math.pyx and _routines_statistics.pyx instead. The duplicate logic could potentially be refactored into a common function

_get_axes will have already sorted the axes in ascending order and checked their range (It also separates out the axes to be reduced from those needed in determining the shape for keepdims).
This allows greatly simplifying the code in _cub_device_segmented_reduce_axis_compatible and removes the need for _cub_device_reduce_axis_compatible altogether.

Although not strictly part of the bug fix, I implemented a performance improvement in the segmented reduction case by replacing the following slow call

numpy.all(numpy.diff(cub_axis) == 1)

with a call to a new _contig_axis Cython helper function

@grlee77
Copy link
Copy Markdown
Member Author

grlee77 commented Nov 27, 2019

Benchmarks regarding the use of _contig_axes over the numpy calls:

import numpy as np
from cupy.cuda.cub import _contig_axes
axes=(1, 2)
%timeit np.all(np.diff(axes) == 1)

6.73 µs ± 75 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

# alternative Python-based solution considered, but not implemented in this PR
%timeit all((ax - ax_prev) == 1 for ax, ax_prev in zip(axes[1:], axes[:-1]))

435 ns ± 1.19 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

%timeit _contig_axes(axes)

39.4 ns ± 0.238 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

leofang
leofang requested changes Nov 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @grlee77. I think you took a harder route by doing some code refactoring, but it's probably the right thing to do. I left a few comments for you.

@grlee77
Copy link
Copy Markdown
Member Author

grlee77 commented Nov 29, 2019

In a follow-up PR, we can easily add a CUB-based path for mean as well:

cdef ndarray _ndarray_mean(ndarray self, axis, dtype, out, keepdims):
    if cupy.cuda.cub_enabled and self.size != 0:
        result = cub.cub_reduction(self, cub.CUPY_CUB_SUM, axis, out, dtype,
                                   keepdims)
        if result is not None:
            result /= (self.size / result.size)
            return result
    return _mean(self, axis=axis, dtype=dtype, out=out, keepdims=keepdims)

@grlee77 grlee77 mentioned this pull request Nov 29, 2019
Closed
10 tasks
leofang
leofang requested changes Nov 30, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

LGTM except for a few cleanups and nitpicks. The new refactoring makes it more readable and maintainable!

@emcastillo emcastillo added st:awaiting-author Awaiting response from author cat:bug Bugs to-be-backported Pull-requests to be backported to stable branch labels Dec 16, 2019
@grlee77
Copy link
Copy Markdown
Member Author

grlee77 commented Dec 18, 2019

Thanks for reviewing @leofang. I had forgotten to get back to this one. It has been rebased and should be ready for testing now.

@grlee77 grlee77 mentioned this pull request Dec 18, 2019
Closed
@emcastillo
Copy link
Copy Markdown
Member

emcastillo commented Dec 19, 2019

Jenkins, test this please

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

pfn-ci-bot commented Dec 19, 2019

Successfully created a job for commit 9497105:

@chainer-ci
Copy link
Copy Markdown
Member

chainer-ci commented Dec 19, 2019

Jenkins CI test (for commit 9497105, target branch master) succeeded!

@emcastillo
Copy link
Copy Markdown
Member

emcastillo commented Dec 23, 2019

Jenkins, test this please

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

pfn-ci-bot commented Dec 23, 2019

Successfully created a job for commit a4bba42:

@emcastillo emcastillo removed the st:awaiting-author Awaiting response from author label Dec 23, 2019
@chainer-ci
Copy link
Copy Markdown
Member

chainer-ci commented Dec 23, 2019

Jenkins CI test (for commit a4bba42, target branch master) succeeded!

@emcastillo emcastillo merged commit ab3fd8b into cupy:master Dec 23, 2019
emcastillo pushed a commit to emcastillo/cupy that referenced this pull request Dec 23, 2019
Merge pull request cupy#2725 from grlee77/cub_keepdims_fix
f54b112 
keepdims should always preserve all dimensions in CUB-based reductions
@emcastillo emcastillo mentioned this pull request Dec 23, 2019
Merged
@grlee77 grlee77 mentioned this pull request Dec 23, 2019
Merged
@leofang leofang mentioned this pull request Jan 7, 2020
Merged
@asi1024 asi1024 added this to the v8.0.0a1 milestone Jan 16, 2020
@grlee77 grlee77 deleted the cub_keepdims_fix branch September 9, 2020 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leofang leofang leofang requested changes

Assignees

@emcastillo emcastillo

Labels

cat:bug Bugs to-be-backported Pull-requests to be backported to stable branch

Projects

None yet

Milestone

v8.0.0a1

Development

Successfully merging this pull request may close these issues.

inconsistent behavior of keepdims for CUB-based reductions

6 participants

@grlee77 @emcastillo @pfn-ci-bot @chainer-ci @leofang @asi1024