C++ gradient for Select by facaiy · Pull Request #14862 · tensorflow/tensorflow

facaiy · 2017-11-24T11:24:50Z

Fix #14845

migrate python implementation to c++ side, source:

tensorflow/tensorflow/python/ops/math_grad.py

Line 919 in 27767d8

@ops.RegisterGradient("Select")

How to test

add test case
pass all tests

tensorflow-jenkins · 2017-11-24T11:24:51Z

Can one of the admins verify this patch?

facaiy · 2017-11-24T11:28:12Z

@suharshs Hi, since Select seems renamed to Where3, I'm a little confused about which op the gradient should be registered for, Select or Where3? Could you give an advice? Thank you.

suharshs · 2017-12-15T23:37:19Z

Sorry for the delay, was on vacation :)

I believe at the kernel level, the op is still "Select" so you should register it for that.

suharshs

a couple minor comments. Thanks!

suharshs · 2017-12-15T23:40:28Z

+  auto zeros = ZerosLike(scope, x);
+  auto grad = grad_inputs[0];
+
+  Scope grad_scope = scope.WithControlDependencies(grad);


Why do you need this control dependency? Isn't there a data dependency with grad already?

suharshs · 2017-12-15T23:42:22Z

 }

+TEST_F(NaryGradTest, Select) {
+  TensorShape x_shape({3, 4});


Nit: Since x_shape is the same as y_shape, maybe rename one to 'shape' and use that everywhere?

facaiy · 2017-12-16T02:20:59Z

Welcome back, @suharshs . I have removed grad_scope, x_shape, y_shape as requested.

yifeif · 2017-12-20T00:40:37Z

@tensorflow-jenkins test this please

gunan · 2018-01-01T05:37:25Z

Correction, this is the cause of the flakes. reverting this PR instead.

This reverts commit dc355dc.

facaiy · 2018-01-02T01:09:37Z

+  TensorShape shape({3, 4});
+  auto x1 = Placeholder(scope_, DT_FLOAT, Placeholder::Shape(shape));
+  auto x2 = Placeholder(scope_, DT_FLOAT, Placeholder::Shape(shape));
+  auto y = Where3(scope_, Greater(scope_, x1, x2), x1, x2);


@gunan @drpngx There seems something wrong with the condition x1 > x2, which might be unstable for gradient calculation. I think that's why tests failed.

@drpngx I want to confirm that: Is NaryGradTest.Select the only one failed test?

I can't remember on the top of my head. I thought there was another one. You can try bazel test --runs_per_test=100 and you should find it.

Fine, I'll look into it later. Thanks for your quick reply, @drpngx .

I have opened #15807 to fix the unstable test case. Might you take a look? Thanks.

…ow#15764)" This reverts commit 4c19f77.

* Revert "Revert "C++ gradient for Select (#14862)" (#15764)" This reverts commit 4c19f77.

…her/scatter. Imported from GitHub PR openxla/xla#14862 Issue #13304 In SPMD handling of gather/scatter the partition strategy is hardcoded to IndexParallel strategy. This is not optimal for all topology. This PR makes this option an SPMD config, but defaults to IndexParallel to maintain existing behavior. Clang-format also fixed some formatting. Tests were added and all tests pass. Copybara import of the project: -- 7f83c21573f24cd4e314b13ce2e349dd6194b451 by ptoulme-aws <[email protected]>: Add SPMD config option to specify zero cost method for gather/scatter. Merging this change closes #14862 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#14862 from ptoulme-aws:gather_scatter_config 7f83c21573f24cd4e314b13ce2e349dd6194b451 PiperOrigin-RevId: 652473292

…her/scatter. Imported from GitHub PR openxla/xla#14862 Issue #13304 In SPMD handling of gather/scatter the partition strategy is hardcoded to IndexParallel strategy. This is not optimal for all topology. This PR makes this option an SPMD config, but defaults to IndexParallel to maintain existing behavior. Clang-format also fixed some formatting. Tests were added and all tests pass. Copybara import of the project: -- 7f83c21573f24cd4e314b13ce2e349dd6194b451 by ptoulme-aws <[email protected]>: Add SPMD config option to specify zero cost method for gather/scatter. Merging this change closes #14862 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#14862 from ptoulme-aws:gather_scatter_config 7f83c21573f24cd4e314b13ce2e349dd6194b451 PiperOrigin-RevId: 652514718

…her/scatter. Imported from GitHub PR openxla/xla#14862 Issue #13304 In SPMD handling of gather/scatter the partition strategy is hardcoded to IndexParallel strategy. This is not optimal for all topology. This PR makes this option an SPMD config, but defaults to IndexParallel to maintain existing behavior. Clang-format also fixed some formatting. Tests were added and all tests pass. Copybara import of the project: -- 7f83c21573f24cd4e314b13ce2e349dd6194b451 by ptoulme-aws <[email protected]>: Add SPMD config option to specify zero cost method for gather/scatter. Merging this change closes #14862 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#14862 from ptoulme-aws:gather_scatter_config 7f83c21573f24cd4e314b13ce2e349dd6194b451 PiperOrigin-RevId: 652473292

…her/scatter. Imported from GitHub PR openxla/xla#14862 Issue #13304 In SPMD handling of gather/scatter the partition strategy is hardcoded to IndexParallel strategy. This is not optimal for all topology. This PR makes this option an SPMD config, but defaults to IndexParallel to maintain existing behavior. Clang-format also fixed some formatting. Tests were added and all tests pass. Copybara import of the project: -- 7f83c21573f24cd4e314b13ce2e349dd6194b451 by ptoulme-aws <[email protected]>: Add SPMD config option to specify zero cost method for gather/scatter. Merging this change closes #14862 PiperOrigin-RevId: 652736743

googlebot added the cla: yes label Nov 24, 2017

facaiy mentioned this pull request Nov 24, 2017

No gradient defined for op: Select #14845

Closed

drpngx requested review from skye and suharshs and removed request for skye December 4, 2017 03:13

drpngx added the awaiting review Pull request awaiting review label Dec 4, 2017

facaiy force-pushed the ENH/add_select_grad_on_c branch from cd2bf45 to 88a2671 Compare December 10, 2017 07:20

facaiy added 2 commits December 10, 2017 15:22

ENH: add SelectGrad

dd7eedc

TST: add test case

172dcec

facaiy force-pushed the ENH/add_select_grad_on_c branch from 88a2671 to 172dcec Compare December 10, 2017 07:22

TST: clean code

4e0621f

suharshs suggested changes Dec 15, 2017

View reviewed changes

facaiy added 2 commits December 16, 2017 10:17

CLN: grad_scope -> scope

ee1fd27

CLN: x_shape, y_shape -> shape

3efe6f3

suharshs approved these changes Dec 18, 2017

View reviewed changes

suharshs removed the awaiting review Pull request awaiting review label Dec 18, 2017

yifeif added awaiting testing (then merge) kokoro:force-run Tests on submitted change labels Dec 20, 2017

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 20, 2017

yifeif added the kokoro:force-run Tests on submitted change label Dec 20, 2017

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 20, 2017

yifeif merged commit dc355dc into tensorflow:master Dec 20, 2017

facaiy deleted the ENH/add_select_grad_on_c branch December 20, 2017 22:42

gunan mentioned this pull request Jan 1, 2018

Revert "add c++ gradient for op: Pow (#15245)" #15761

Closed

drpngx pushed a commit that referenced this pull request Jan 1, 2018

Revert "C++ gradient for Select (#14862)" (#15764)

4c19f77

This reverts commit dc355dc.

facaiy commented Jan 2, 2018

View reviewed changes

facaiy added a commit to facaiy/tensorflow that referenced this pull request Jan 3, 2018

Revert "Revert "C++ gradient for Select (tensorflow#14862)" (tensorfl…

a3b778c

…ow#15764)" This reverts commit 4c19f77.

facaiy mentioned this pull request Jan 3, 2018

Fix unstable test case for Select op #15807

Merged

caisq pushed a commit that referenced this pull request Jan 4, 2018

Fix unstable test case for Select op (#15807)

6db014b

* Revert "Revert "C++ gradient for Select (#14862)" (#15764)" This reverts commit 4c19f77.

gundamMC mentioned this pull request Apr 24, 2018

No gradient defined for op: Select migueldeicaza/TensorFlowSharp#264

Closed

copybara-service Bot mentioned this pull request Jul 15, 2024

PR #14862: Add SPMD config option to specify zero cost method for gather/scatter. #71862

Closed

copybara-service Bot mentioned this pull request Jul 15, 2024

PR #14862: Add SPMD config option to specify zero cost method for gather/scatter. #71868

Merged

Conversation

facaiy commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test

Uh oh!

tensorflow-jenkins commented Nov 24, 2017

Uh oh!

facaiy commented Nov 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suharshs commented Dec 15, 2017

Uh oh!

suharshs left a comment

Choose a reason for hiding this comment

Uh oh!

suharshs Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

suharshs Dec 15, 2017

Choose a reason for hiding this comment

Uh oh!

facaiy commented Dec 16, 2017

Uh oh!

yifeif commented Dec 20, 2017

Uh oh!

gunan commented Jan 1, 2018

Uh oh!

facaiy Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facaiy Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

drpngx Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

facaiy Jan 2, 2018

Choose a reason for hiding this comment

Uh oh!

facaiy Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

facaiy commented Nov 24, 2017 •

edited

Loading

facaiy commented Nov 24, 2017 •

edited

Loading

facaiy Jan 2, 2018 •

edited

Loading