Skip to content

Merge reduction implementations#2732

Merged
asi1024 merged 13 commits intocupy:masterfrom
niboshi:refactor-reduction
Dec 5, 2019
Merged

Merge reduction implementations#2732
asi1024 merged 13 commits intocupy:masterfrom
niboshi:refactor-reduction

Conversation

@niboshi
Copy link
Copy Markdown
Member

@niboshi niboshi commented Nov 28, 2019

Merges the logic of ReductionKernel and simple_reduction_function.

@niboshi niboshi force-pushed the refactor-reduction branch 2 times, most recently from bfcbc62 to eca27aa Compare November 28, 2019 20:17
@asi1024 asi1024 self-assigned this Nov 29, 2019
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.

Thank you for this effort, @niboshi! I've had a hard time understanding the duplicate reduction routines, and this makes it a bit cleaner. I left one quick question.

@asi1024 asi1024 added the cat:code-fix Code refactoring that do not change behavior label Dec 2, 2019
@niboshi
Copy link
Copy Markdown
Member Author

niboshi commented Dec 2, 2019

PTAL.

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 2, 2019

Some parts of this PR are duplicate to #2697. I will review this PR after #2697 is merged.

@okuta
Copy link
Copy Markdown
Member

okuta commented Dec 2, 2019

This is good PR.
It is OK to merge before # 2697.

out_block_num * block_size, inout_args, 0, block_size, stream)
return ret

cpdef _get_expressions_and_types(self, in_args, out_args, dtype):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not support them as cdef methods and write argument types explicitly?

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.

cdef methods cannot be overridden.
dtype argument does not have a specific type.

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.

Do you mean other arguments?
I doubt typing tuples improves performance. I'll check.

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.

I only observe difference of 1-2 ns. I think it's negligible.

niboshi and others added 3 commits December 3, 2019 11:33
Co-Authored-By: Akifumi Imanishi <akifumi.imanishi@gmail.com>
Co-Authored-By: Ryosuke Okuta <okuta@preferred.jp>
@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 3, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit 1248a78:

@asi1024 asi1024 added this to the v7.0.0 milestone Dec 3, 2019
@niboshi niboshi force-pushed the refactor-reduction branch from 1248a78 to 79793ee Compare December 3, 2019 13:25
@niboshi
Copy link
Copy Markdown
Member Author

niboshi commented Dec 3, 2019

Force-pushed a fix to remove an incorrect typing to axis argument.

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 3, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit 79793ee:

@chainer-ci
Copy link
Copy Markdown
Member

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

@chainer-ci
Copy link
Copy Markdown
Member

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

@niboshi niboshi force-pushed the refactor-reduction branch from aecac39 to 16be409 Compare December 4, 2019 07:31
@niboshi
Copy link
Copy Markdown
Member Author

niboshi commented Dec 4, 2019

Conflict resolved.

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 4, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit 16be409:

@chainer-ci
Copy link
Copy Markdown
Member

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

@niboshi
Copy link
Copy Markdown
Member Author

niboshi commented Dec 4, 2019

Fixed test failure.
PTAL.

@asi1024 asi1024 removed this from the v7.0.0 milestone Dec 5, 2019
@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 5, 2019

Jenkins, test this please.

@pfn-ci-bot
Copy link
Copy Markdown
Collaborator

Successfully created a job for commit 07778d9:

@asi1024 asi1024 added this to the v7.1.0 milestone Dec 5, 2019
@chainer-ci
Copy link
Copy Markdown
Member

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

@asi1024
Copy link
Copy Markdown
Member

asi1024 commented Dec 5, 2019

LGTM.

@asi1024 asi1024 merged commit b232b86 into cupy:master Dec 5, 2019
@niboshi niboshi deleted the refactor-reduction branch December 5, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:code-fix Code refactoring that do not change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants