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 issues in official tf.nn.topk() in lite #18896

Merged
merged 1 commit into from Apr 26, 2018

Conversation

scottcjt
Copy link
Contributor

Not sure if Google is planning a breaking change. If not:

  1. The topk_v2 implementation has its output values and indices in the opposite order.

    Below is TensorFlow Python API behavior:

    In [5]: x
    Out[5]: <tf.Tensor 'x:0' shape=(100,) dtype=float32>
    
    In [7]: y, z = tf.nn.top_k(x, k=10)
    
    In [8]: y
    Out[8]: <tf.Tensor 'TopKV2:0' shape=(10,) dtype=float32>
    
    In [10]: z
    Out[10]: <tf.Tensor 'TopKV2:1' shape=(10,) dtype=int32>
  2. Output array types are not correctly propagated.

  3. First output array name should not be appended with ":0" because it is deliberately removed here.

  4. If k is provided at run time, two output arrays will be dynamic, and have shape [..., 0] here. I changed toco to only check constant buffers instead, RFC.

@andrehentz andrehentz added the kokoro:force-run Tests on submitted change label Apr 26, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2018
@andrehentz andrehentz added the kokoro:force-run Tests on submitted change label Apr 26, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 26, 2018
@ekelsen ekelsen merged commit d0b9613 into tensorflow:master Apr 26, 2018
@scottcjt scottcjt deleted the fix_lite_topk branch April 27, 2018 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants