Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement chainerx::SoftmaxCrossEntropy and chainerx.softmax_cross_entropy #8250

Merged
merged 13 commits into from Oct 23, 2019
Merged

Implement chainerx::SoftmaxCrossEntropy and chainerx.softmax_cross_entropy #8250

merged 13 commits into from Oct 23, 2019

Conversation

takagi
Copy link
Member

@takagi takagi commented Oct 6, 2019 •
edited

This PR implements chainerx::SoftmaxCrossEntropy and chainerx.softmax_cross_entropy, as well as replacing its temporary implementations in Chainer inter-op and ChainerX MNIST example.

Limitations:

  • The number of dimensions of x1 is two only
  • No reduction mode
  • ignore_label can not be specified
  • No cache_score, class_weight and enable_double_backprop arguments that F.softmax_cross_entropy has

I fixed F. SoftmaxCrossEntropy's forward_chainerx() method to use chainerx.softmax_cross_entropy as well.

I will fix chainer/chainerx_cc/examples/mnist/ and chainer/chainerx_cc/examples/mnist_py/ to use the new ChainerX implementation in another PR.

@takagi takagi changed the title Implement chainerx::SoftmaxCrossEntropy and chainerx.softmax_cross_entropy Implement chainerx::SoftmaxCrossEntropy and chainerx.softmax_cross_entropy Oct 6, 2019
hvy
hvy requested changes Oct 7, 2019
View reviewed changes
chainerx_cc/chainerx/routines/loss.h Outdated Show resolved Hide resolved
@hvy hvy added cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. labels Oct 7, 2019
@takagi takagi force-pushed the chainerx-softmax-cross-entropy branch from aa91516 to 6138818 Compare October 7, 2019 08:19
@takagi takagi marked this pull request as ready for review October 10, 2019 07:27
@takagi takagi force-pushed the chainerx-softmax-cross-entropy branch from 3d569ab to baf3f98 Compare October 10, 2019 07:40
@takagi
Copy link
Member Author

takagi commented Oct 10, 2019

@Hyv I've made this PR ready with updating PR description. Would you start reviewing?

@takagi takagi mentioned this pull request Oct 15, 2019
Merged
hvy
hvy requested changes Oct 17, 2019
View reviewed changes
tests/chainerx_tests/unit_tests/routines_tests/test_loss.py Outdated
@@ -172,6 +172,42 @@ def forward_chainer(self, inputs):
return out,


@op_utils.op_test(['native:0'])
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add 'cuda:0'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

chainer/functions/loss/softmax_cross_entropy.py Outdated
@@ -100,7 +100,7 @@ def _is_chainerx_supported(self, input_arrays):
return False
if self.ignore_label != -1:
return False
if self.reduce != 'mean':
if self.reduce == 'mean' and self.normalize:
return False
Copy link
Member

Choose a reason for hiding this comment

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

These are the default configurations. How about instead of falling back, run the previous implementation in this case? The denominator https://github.com/chainer/chainer/pull/8250/files#diff-7f9fda8b243258115ef7bdac145fec8dL122 should be mask.sum() instead of x.shape[0] though.

Copy link
Member

Choose a reason for hiding this comment

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

There was a bug with normalize which I fixed in #8301. Maybe we should wait for that one to be merged before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. I'll review #8301.

@hvy hvy mentioned this pull request Oct 17, 2019
Merged
2 tasks
@hvy
Copy link
Member

hvy commented Oct 18, 2019

@takagi thanks for reviewing #8301. Could you resolve the conflicts in this PR that now occurred?

@takagi
Copy link
Member Author

takagi commented Oct 19, 2019

Resolved the conflict!

@hvy
Copy link
Member

hvy commented Oct 21, 2019

Sorry one last suggestion #8250 (comment).

@hvy
Copy link
Member

hvy commented Oct 23, 2019

Jenkins, test this please.

@hvy
Copy link
Member

hvy commented Oct 23, 2019

LGTM!

I'm thinking if we should introduce arguments after all similar to SCE in Chainer, simply to make it more useful. But this also applied to SigmoidCrossEntropy so we should probably address them both in the discussion.

@hvy hvy added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Oct 23, 2019
@hvy hvy added this to the v7.0.0rc1 milestone Oct 23, 2019
@chainer-ci
Copy link
Member

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

@mergify mergify bot merged commit af0f3ad into chainer:master Oct 23, 2019
@takagi takagi deleted the chainerx-softmax-cross-entropy branch October 23, 2019 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hvy hvy hvy approved these changes

Assignees

@hvy hvy

Labels
cat:feature Implementation that introduces new interfaces. ChainerX Related to ChainerX. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Milestone
v7.0.0rc1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@takagi @hvy @chainer-ci