Skip to content

Conversation

@annxingyuan
Copy link
Contributor

@annxingyuan annxingyuan commented Jun 22, 2020

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


This change is Reviewable

@annxingyuan annxingyuan requested a review from pyu10055 June 22, 2020 16:04
Copy link
Collaborator

@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: 0 of 1 approvals obtained (waiting on @annxingyuan and @pyu10055)


tfjs-core/src/ops/slice_util.ts, line 118 at r1 (raw file):

        newIndices[ellipsisInsertionIndex] = 0;
      } else {
        newIndices.splice(

Since you are iterating from low to high, I am curious why you need to insert and pop from back, can you replace the value at the index with 0?


tfjs-core/src/ops/strided_slice.ts, line 96 at r1 (raw file):

  // Normalize the start, end and strides.
  if (ellipsisAxes.length && numInterpolatedAxes > 0) {

when ellipsis mask is not zero, will the condition be not true?
meaning if the startForAxis and stopForAxis method need the ellipsisMask as parameter?

Copy link
Collaborator

@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.

Thank you for fixing this.

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan and @pyu10055)

Copy link
Contributor Author

@annxingyuan annxingyuan 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: 0 of 1 approvals obtained (waiting on @pyu10055)


tfjs-core/src/ops/slice_util.ts, line 118 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Since you are iterating from low to high, I am curious why you need to insert and pop from back, can you replace the value at the index with 0?

Done

Thanks for catching this - this logic was leftover from the previous implementation, when newIndices were initialized to begin / end.


tfjs-core/src/ops/strided_slice.ts, line 96 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

when ellipsis mask is not zero, will the condition be not true?
meaning if the startForAxis and stopForAxis method need the ellipsisMask as parameter?

Sometimes ellipsisAxis.length > 0 but numInterpolatedAxes = 0, this is the case when the ellipsis only applies to one axis because begin / end have the same dimensionality as the input shape.

@annxingyuan annxingyuan requested a review from pyu10055 June 23, 2020 12:44
Copy link
Collaborator

@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.

THanks!

@annxingyuan annxingyuan merged commit ae3641a into master Jun 25, 2020
@annxingyuan annxingyuan deleted the ellipsis_ss branch June 25, 2020 18:00
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