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.

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

Fix some inconsistency in gradient variable names. #4331

Merged
merged 3 commits into from Feb 16, 2018
Merged

Fix some inconsistency in gradient variable names. #4331

merged 3 commits into from Feb 16, 2018

Conversation

unageek
Copy link
Contributor

@unageek unageek commented Feb 14, 2018

According to the source code and the slides about double backprop., it seems a common practice to use gx and ggx to represent the gradients wrt x and gx.

However, some variables have contradictory names to this convention and new users may get confused about it. I hope they will be fixed by this PR.

@toslunar
Copy link
Member

The Travis failure seems unrelated to the PR.

@okuta
Copy link
Member

okuta commented Feb 15, 2018

jenkins, test this please.

@okuta okuta assigned toslunar and unassigned delta2323 Feb 15, 2018
Copy link
Member

@toslunar toslunar 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 the PR. I left two comments.

y_mul_ggx = y * ggx
gx = -2 * gy * y_mul_ggx
ggy = ggx - y * y_mul_ggx
return gx, ggy
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep the name grad_y, since it's not a gradient of x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Oh, TanhGrad is defined as a function of y, not x!

g, = grad_outputs
return LinearInterpolateGrad().apply((p, x, y, g))
gz, = grad_outputs
return LinearInterpolateGrad().apply((p, x, y, gz))


class LinearInterpolateGrad(function_node.FunctionNode):
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to use the same name for an input in forward and backward. (g vs gz)

@unageek
Copy link
Contributor Author

unageek commented Feb 16, 2018

Thank you for your review. I've made some changes according to your comments. Please confirm.

@toslunar
Copy link
Member

jenkins, test this please

@okuta okuta added cat:enhancement Implementation that does not break interfaces. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Feb 16, 2018
Copy link
Member

@toslunar toslunar left a comment

Choose a reason for hiding this comment

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

LGTM

@toslunar toslunar merged commit c2ad311 into chainer:master Feb 16, 2018
@niboshi niboshi added this to the v4.0.0b4 milestone Feb 19, 2018
@unageek unageek deleted the fix-var-names branch February 20, 2018 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants