Skip to content

Fix _reduce_dims call in reduction#3262

Merged
mergify[bot] merged 3 commits intocupy:masterfrom
emcastillo:fix_reduction
Apr 7, 2020
Merged

Fix _reduce_dims call in reduction#3262
mergify[bot] merged 3 commits intocupy:masterfrom
emcastillo:fix_reduction

Conversation

@emcastillo
Copy link
Copy Markdown
Member

@emcastillo emcastillo commented Apr 6, 2020

Closes #3258

When calling _reduce_dims, the out_args tuple was empty, so dimensions were never reduced.
_get_out_args is the function that initializes the actual arrays if the out parameter is an empty list, and it wasn't called until later.

This PR calls _get_out_args before and then retrieves its ndarray objects, so that dimensions can be effectively reduced. Then it recreates the output Args as the dimensionality might have changed and the kernel generated code can be wrong.

Another way to solve this could be to make _reduce_dims allow a list of Arg objects as the parameter and modify it accordingly.

ElementwiseKernel does not suffer from this issue

@emcastillo emcastillo changed the title Fix _reduce_dims call in reduction [WIP] Fix _reduce_dims call in reduction Apr 6, 2020
@emcastillo emcastillo changed the title [WIP] Fix _reduce_dims call in reduction Fix _reduce_dims call in reduction Apr 6, 2020
@emcastillo
Copy link
Copy Markdown
Member Author

Fixed, PTAL

out_shape = _get_out_shape(a_shape, reduce_axis, out_axis, keepdims)
# Needs to create the empty arrays first and access them for the
# _reduce_dims to work.
out_args_ = self._get_out_args(out_args, out_types, out_shape)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is where the actual out arrays are instantiated when out=None

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 6, 2020

Could you add a test for this case?

@asi1024 asi1024 added the cat:bug Bugs label Apr 6, 2020
@asi1024 asi1024 added this to the v8.0.0b2 milestone Apr 6, 2020
@emcastillo
Copy link
Copy Markdown
Member Author

The behavior is not exposed externally, so It's hard to test.
The only approach I can think is to add the case that failed, (long number of dims)

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 6, 2020

It can be tested with a mock test, but I think it is OK to do it in another PR.

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 6, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit f2bb128:

@emcastillo
Copy link
Copy Markdown
Member Author

Sorry, I just pushed a test based on the original error now 😆
I verified that this test fails in the master branch.

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 6, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit eb88eab:

@asi1024 asi1024 added the st:test-and-merge (deprecated) Ready to merge after test pass. label Apr 6, 2020
@emcastillo
Copy link
Copy Markdown
Member Author

pfnCI is appearing but Jenkins is missing from the checks 😂

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 6, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit eb88eab:

@kmaehashi
Copy link
Copy Markdown
Member

pfnCI, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit eb88eab:

@kmaehashi
Copy link
Copy Markdown
Member

pfnCI, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit eb88eab:

@chainer-ci
Copy link
Copy Markdown
Member

Jenkins CI test (for commit eb88eab, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Copy Markdown
Member Author

Fixed, PTAL

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Apr 7, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit f6e470d:

@kmaehashi
Copy link
Copy Markdown
Member

pfnCI, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit f6e470d:

@chainer-ci
Copy link
Copy Markdown
Member

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

@mergify mergify bot merged commit f1e020c into cupy:master Apr 7, 2020
@emcastillo emcastillo deleted the fix_reduction branch April 7, 2020 05:27
@chainer-ci
Copy link
Copy Markdown
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:bug Bugs st:test-and-merge (deprecated) Ready to merge after test pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduction fails with CUDA_ERROR_LAUNCH_OUT_OF_RESOURCES in some inputs

5 participants