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

Allow squeeze to use negative index#1430

Merged
caisq merged 5 commits into
tensorflow:masterfrom
Kriyszig:negativeval
Dec 10, 2018
Merged

Allow squeeze to use negative index#1430
caisq merged 5 commits into
tensorflow:masterfrom
Kriyszig:negativeval

Conversation

@Kriyszig
Copy link
Copy Markdown
Contributor

@Kriyszig Kriyszig commented Dec 5, 2018

Issue in focus: tensorflow/tfjs#940

Description

squeeze() didn't support the use of negative index which is supported in tensorflow for Python. Added tests for the same in util_test and tensor_test.
The previous implementation didn't allow for random arrangement of values of axis. For example if axes is set to [2, 3], it would remove them both but if you pass it as [3, 2] it would only remove 3 and ignore 2 entirely. I tried the same with tensorflow for python in this Google Colab Notebook and this too was inconsistent with tfjs as tensorflow python allows for removal of axis irrespective of order of input if it's allowed. This inconsistency was removed when adding the support for negative index ( it was totally unintentional but resulted from my implementation to allow negative index ).

Clarification required for following topics:

  • I am not sure whether the test messages are satisfactory for the new tests added - Please let me know if this can be improved and I'll implement the same as soon as possible.
  • The random input order support was a result of my judgement where i thought [-1, -2] was intuitively much better for a programmer but if we just replaced it with the positive index of same, it would have been [5, 4] which due to it's lack of ascending order would have ignored 4 completely. If this needs to changed, please let me know and I'll come up with another implementation to change this behavior.

Tests Ran:

  • yarn test
  • yarn lint

BUG


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: BUG

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


This change is Reviewable

Body:
BUG

* squeeze() can now take negative index where -1 refers to the index (length - 1) thus extending the range of axis parameter from [0, length) to [-length, length) making it consistent with the python version of tensorflow.
* Added tests for the same in util_tests and tensor_tests
@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@Kriyszig
Copy link
Copy Markdown
Contributor Author

Kriyszig commented Dec 5, 2018

I signed it!

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

squeeze() will now throw an exception if the index of axis that needs to be squeezed goes out of bound
@Kriyszig
Copy link
Copy Markdown
Contributor Author

Kriyszig commented Dec 6, 2018

  • Added an exception case in case the given value of axis doesn't lie in [ -shape.length, shape.length) which further makes it consistent with tensorflow for python.
    The behavior of Python in the case can be found in this Google Colab Notebook

Seeking Clarification on following issue:

  • The error message was more than 80 characters long and I suppressed TSist using disable-line. Please let me know if this can be replaced with a shorted message, or another approach altogether

Tests Ran:

  • yarn test
  • yarn lint

Copy link
Copy Markdown
Collaborator

@caisq caisq 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 @VisibleMarkov)


src/tensor_test.ts, line 1361 at r2 (raw file):

[-1]

Can you also test squeeze([-2, -1])?


src/util.ts, line 282 at r2 (raw file):

`Can't squeeze axis ${axis[i]} since its not in [-${shape.length}, ${shape.length}) for shape ${shape}`); // tslint:disable-line

You can break this into two lines by using the string + operator and remove the tslint disable comment.

@Kriyszig
Copy link
Copy Markdown
Contributor Author

Kriyszig commented Dec 9, 2018

@caisq Addressed the changes requested in 02d2344
If anything needs to be changed, please let me know and I'll implement it as soon as I can (^_^)

Copy link
Copy Markdown
Collaborator

@caisq caisq left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: 0 of 1 approvals obtained (waiting on @VisibleMarkov)

@Kriyszig
Copy link
Copy Markdown
Contributor Author

Thanks for the green flag (^_^)
Just curious: Should I wait for another review by someone else?

@caisq caisq merged commit 5c0b5a7 into tensorflow:master Dec 10, 2018
@caisq
Copy link
Copy Markdown
Collaborator

caisq commented Dec 10, 2018

@VisibleMarkov PR is merged. Thank you for your contribution!

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