Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Add gradients for tf.min and tf.max #1272

Merged
merged 10 commits into from
Sep 6, 2018
Merged

Add gradients for tf.min and tf.max #1272

merged 10 commits into from
Sep 6, 2018

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Sep 6, 2018

Fixes tensorflow/tfjs#677

This change is Reviewable

@caisq caisq changed the title [WIP] Add gradients for tf.min and tf.max Add gradients for tf.min and tf.max Sep 6, 2018
@caisq caisq requested a review from nsthorat September 6, 2018 17:15
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @nsthorat)


src/ops/reduction_ops.ts, line 223 at r1 (raw file):

  return {
    $x: () => {
      const dx = dy.mul(xOrig.equal(y).cast(dy.dtype));

dy.dtype is always float32


src/ops/reduction_ops_test.ts, line 200 at r1 (raw file):

      [[[0, 20], [20, 20]], [[-10, -30], [-10, -30]]]
    ]);
    const dy = tf.tensor4d([[[[-1]], [[-1]]], [[[-1]], [[-1]]]]);

small nit, can you make the values of dy be -1, -2, -3, -4 etc here and above? this will ensure that the first value isn't being placed everywhere

Copy link
Collaborator Author

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq)


src/ops/reduction_ops.ts, line 223 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

dy.dtype is always float32

It seems to be less magical and potentially more maintainable to use dy.dtype here. Also, I don't think that call is particularly expensive.


src/ops/reduction_ops_test.ts, line 200 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

small nit, can you make the values of dy be -1, -2, -3, -4 etc here and above? this will ensure that the first value isn't being placed everywhere

Done.

@caisq caisq merged commit a065dbc into tensorflow:master Sep 6, 2018
@caisq caisq deleted the max-grad branch September 6, 2018 19:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants