Skip to content

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Aug 14, 2019

  • Add support for the newAxisMask param in stridedSlice(). Because stridedSlice() is difficult to implement correctly, and all backends have to repeat fragile logic, shift common preprocess and postprocess logic to the logical stridedSlice op and simplify the actual kernel.
  • Make tf.squeeze(x, axis) consistent with TF when axis is empty array.

These fixes were found by running a converted TF model in the browser.


This change is Reviewable

@dsmilkov dsmilkov changed the title Support newAxisMask in stridedSlice and make squeeze consistent with TF Py [tfjs-core] Support newAxisMask in stridedSlice and make squeeze consistent with TF Py Aug 14, 2019
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 9 of 9 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @pyu10055)


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

    const ellipsisMask = 0;
    const newAxisMask = 10;     // 1010
    const shrinkAxisMask = 20;  // 10100

you can use his here and elsewhere

0b10100

Copy link
Contributor Author

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pyu10055)


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

Previously, nsthorat (Nikhil Thorat) wrote…

you can use his here and elsewhere

0b10100

Done

@dsmilkov dsmilkov merged commit aa92053 into master Aug 15, 2019
@dsmilkov dsmilkov deleted the fix-strided-slice branch August 15, 2019 18:44
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.

4 participants