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

Add support for ellipsisMask in stridedSlice. #3304

Merged
merged 16 commits into from
May 28, 2020
Merged

Add support for ellipsisMask in stridedSlice. #3304

merged 16 commits into from
May 28, 2020

Conversation

annxingyuan
Copy link
Collaborator

@annxingyuan annxingyuan commented May 21, 2020

StridedSlice with ellipsisMask is used by certain handwriting recognition models.

This fixes #2892

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


This change is Reviewable

@annxingyuan annxingyuan changed the title WIP Ellipsis in stridedSlice Add support for ellipsisMask in stridedSlice. May 26, 2020
@annxingyuan annxingyuan self-assigned this May 26, 2020
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @nsthorat, and @pyu10055)


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

// Creates full selection at the ellided dimensions. If the dimension matches
// the ellipsis mask, override the current start value. Otherwise, insert.
export function startIndicesWithEllidedDims(

nit: s/ellided/elided here and elsewhere

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: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @pyu10055)


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

          ellipsisInsertionIndex, 0 /* num elements to delete */,
          1 /* element to add */);
      newStrides.pop();

I am curious why you need to pop one?

Copy link
Collaborator 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: :shipit: complete! 2 of 1 approvals obtained (waiting on @dsmilkov and @pyu10055)


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

Previously, pyu10055 (Ping Yu) wrote…

I am curious why you need to pop one?

Wherever we insert the ellipsis, it pushes the explicit selections that come after the ellipsis (in case the explicit selection did not fill the rank of the input).

So if our explicit selection looks like this: [1:2, 2:3, 3:4] with ellipsisMask = 2 and the input rank is 5, then after normalization our selection will be turned to this: [1:2,:, 3:4, :, :]. However if we insert an ellipsis at position 1 then we need to push out the implicit full selection at positions 3 and 4 so the final selection looks like this: [1:2, :, :, :, 3:4].


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

Previously, nsthorat (Nikhil Thorat) wrote…

nit: s/ellided/elided here and elsewhere

Done

@annxingyuan annxingyuan merged commit 89c7210 into master May 28, 2020
@annxingyuan annxingyuan deleted the ellipsis branch May 28, 2020 10:15
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.

ellipsis mask is not yet supported
4 participants