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

Fixed squeeze() mutating the arguments passed#1454

Merged
dsmilkov merged 3 commits into
tensorflow:masterfrom
Kriyszig:immutable
Dec 17, 2018
Merged

Fixed squeeze() mutating the arguments passed#1454
dsmilkov merged 3 commits into
tensorflow:masterfrom
Kriyszig:immutable

Conversation

@Kriyszig
Copy link
Copy Markdown
Contributor

@Kriyszig Kriyszig commented Dec 13, 2018

Commit: 5c0b5a7

If the axis is passed to squeeze() as an array, the array mutates if it's not in an ascending order or if it contains negative indexes.

Code to reproduce:

let axis = [-1, -2, -3];
a.squeeze([2, 1, 1, 1], axis)
console.log(axis)
// [1, 2, 3]
// Negative indexes are turned to positive and are sorted

AFAIK, this shouldn't be the behavior.
This slipped in due to my own ignorance in the above mentioned commit. I apologize for the overlook.

@caisq, would love to know your thoughts on this

Description


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

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


This change is Reviewable

Copy link
Copy Markdown
Contributor

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

Thanks for the contributon! Couple of small comments.

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


src/util.ts, line 279 at r1 (raw file):

  const keptDims: number[] = [];
  if (axis != null) {
    axis = axis.map((i) => {

we already have a convenient method parseAxisParam in axis_util.ts which you can call here. Also add a unit test that asserts that the user-provided axis was not modified? Thanks!

VisibleMarkov added 2 commits December 14, 2018 23:21
Previous implementation mutated the argument for axis passed to squeeze()
Used map to make squeeze() immutable
@Kriyszig
Copy link
Copy Markdown
Contributor Author

Kriyszig commented Dec 14, 2018

@dsmilkov, turns out the import of parseAxisParam leads to a dependency loop (although harmless). Any way to get around this? or can I just go back to the map implementation?

@dsmilkov
Copy link
Copy Markdown
Contributor

I see. Move parseAxisParam to util.ts then. You will have to update several imports but the typescript compiler/vscode can help you there. Thanks!

parseAxisParams which caused a dependency loop is moved to util.ts
All the imports and references of the same have been updated
@Kriyszig
Copy link
Copy Markdown
Contributor Author

Kriyszig commented Dec 14, 2018

@dsmilkov
Added the changes requested in d8adc90 - the implementation of parseAxisParam and the tests
Moved parseAxisParam to util.ts in fd0c87c. Updated all the references and imports of same.
Thanks for tip - vscode + Github search helped a lot (^_^)
If there is anything out of order, please let me know and I'll change it as soon as possible

Copy link
Copy Markdown
Contributor

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

Thanks!

Reviewed 9 of 9 files at r3.
Reviewable status: 0 of 1 approvals obtained

@dsmilkov dsmilkov merged commit 172e1cd into tensorflow:master Dec 17, 2018
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.

2 participants