Skip to content

Update PR curve summary op to use tf.one_hot #316

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

Merged
merged 5 commits into from
Aug 2, 2017

Conversation

chihuahua
Copy link
Member

@chihuahua chihuahua commented Aug 2, 2017

Previously, the op had used scatter-related logic that I had introduced as a patch (because scatter_add was unsupported due to tensorflow/tensorflow#11856). This change makes the logic one-hot encode the indices and perform a summation in order to bucket the predictions. This change thus removes variables and control dependencies from the summary op logic, which reduces complexity.

Also, the previous logic to patch scatter_add had been incorrect (scatter_update did not sum up values at the same indices), though the test was too simple to catch that. A subsequent PR will make the tests run off of data generated from a demo and thus make the tests more rigorous.

Previously, they had used scatter-related logic. This change makes the logic one-hot encode the indices and perform a summation in order to bucket the predictions.

This change thus removes variables and control dependencies from the summary op logic, which reduces complexity.
@chihuahua chihuahua requested a review from wchargin August 2, 2017 17:14
@wchargin
Copy link
Contributor

wchargin commented Aug 2, 2017

  • How feasible would it be to make a simple test that exercises the currently-buggy behavior?
  • Good catch on noticing this, but why wasn't it noticed until now if the output was wrong? (Was it only introduced in the tf port, so wasn't actually in production?)
  • The changes mentioned in final paragraph should be in a separate PR.

Will review code in a bit.

@chihuahua
Copy link
Member Author

chihuahua commented Aug 2, 2017

@wchargin,

  • Done. We just need 2 predictions in the same bin.
  • The scatter_add formulation worked. My adaptation using tf.gather did not.
  • Done.

@wchargin
Copy link
Contributor

wchargin commented Aug 2, 2017

Ah, I see: the patch to scatter_add, not scatter_add itself, was the problem. I understand now.

Can you please confirm that the test that you added fails before this commit and passes after it?

@chihuahua
Copy link
Member Author

I updated the PR description to note that the patch had been awry.

The test fails before this commit:

AssertionError: 
Not equal to tolerance rtol=1e-07, atol=0

(mismatch 20.0%)
 x: array([[ 2.      ,  2.      ,  2.      ,  2.      ,  1.      ,  1.      ,
         1.      ,  1.      ,  0.      ,  0.      ],
       [ 2.      ,  2.      ,  1.      ,  1.      ,  1.      ,  1.      ,...
 y: array([[  2.000000e+00,   2.000000e+00,   2.000000e+00,   2.000000e+00,
          1.000000e+00,   1.000000e+00,   0.000000e+00,   0.000000e+00,
          0.000000e+00,   0.000000e+00],...

----------------------------------------------------------------------

and now passes.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

The test fails before this commit: […] and now passes.

Perfect: that's what I wanted to hear. Thanks!

I'm also glad that we've gotten rid of the Variables. Everything is now much easier to reason about.

@chihuahua chihuahua merged commit 8e4e383 into master Aug 2, 2017
@chihuahua chihuahua deleted the one_hot_pr_curve_summary branch August 2, 2017 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants