Skip to content

Conversation

@pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Jun 20, 2020

the numOfSplit param can have -1 value, which indicate the rest of the dimension.
https://www.tensorflow.org/api_docs/python/tf/raw_ops/SplitV

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@pyu10055 pyu10055 requested a review from lina128 June 20, 2020 00:35
Copy link
Collaborator

@lina128 lina128 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 @lina128 and @pyu10055)


tfjs-converter/src/operations/executors/slice_join_executor.ts, line 135 at r1 (raw file):

      if (Array.isArray(numOrSizeSplits)) {
        if (numOrSizeSplits[numOrSizeSplits.length - 1] === -1) {
          const total = numOrSizeSplits.reduce((a, b) => b > 0 ? a + b : a);

nit: From the code, this indicates that any element can be < 0, is that true? If not, can we simply reduce((a, b) => a + b)?

Copy link
Collaborator Author

@pyu10055 pyu10055 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 @lina128)


tfjs-converter/src/operations/executors/slice_join_executor.ts, line 135 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

nit: From the code, this indicates that any element can be < 0, is that true? If not, can we simply reduce((a, b) => a + b)?

Thanks, I updated the logic, the neg value can be anywhere in the array, but there should be just one.

@pyu10055 pyu10055 merged commit bf11b5c into master Jun 21, 2020
@pyu10055 pyu10055 deleted the split_op branch June 21, 2020 23:49
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.

3 participants