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

initial commit diagPart operator #1427

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kedevked
Copy link
Contributor

@kedevked kedevked commented Dec 4, 2018

FEATURE
add operator diagPart

tensorflow/tfjs#655

Description


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

@rthadur rthadur requested a review from dsmilkov March 26, 2019 19:49
@rthadur rthadur self-assigned this Mar 26, 2019
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Sorry for the really long wait. I left comments related to the changed behavior of tf.diagPart. Also if you can sync with master that would be good.

Heads up, we are in the process of switching to a mono-repo, so you can either:

  1. try to finish this PR in the next few days
  2. or, you can reopen the PR in the tensorflow/tfjs repo once we move tfjs-core there.

Reviewed 1 of 7 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @kedevked)


src/kernels/backend_cpu.ts, line 2541 at r1 (raw file):

  diagPart(x: Tensor): Tensor {
    const xVals = x.dataSync();
    const buffer = ops.buffer([Math.sqrt(x.size)], x.dtype);

The size of the output is not necessarily the sqrt(size) since the last output dim is the minimum of the last two input dims: See https://www.tensorflow.org/versions/r2.0/api_docs/python/tf/linalg/diag_part


src/ops/diagpart.ts, line 13 at r1 (raw file):

 * input.
 *
 * Assume the input has dimensions `[D1,..., Dk, D1,..., Dk]`, then the output

This is an outdated behavior of diag_part. See https://www.tensorflow.org/versions/r2.0/api_docs/python/tf/linalg/diag_part for the new definition. For an input tensor of rank k, the output is rank k-1 (in other words, all the dimensions except the last two are treated as a batch)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants