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.

Sign up for GitHub

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

Jump to bottom

[TF] Change APIs to use Int instead of Int32. #24012

Merged
merged 8 commits into from
Apr 16, 2019
Merged

[TF] Change APIs to use Int instead of Int32. #24012

merged 8 commits into from
Apr 16, 2019

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Apr 13, 2019 •
edited
Loading

  • Let TensorShape store [Int].
  • Let Tensor subscripts take Int and Range<Int>.
  • Reduce explicit calls to Int32.init in user code.
    • Notably, initializers taking TensorShape no longer need Int32.init.

Added two shims:

  • Tensor(_: [Int]) creates a Tensor<Scalar>, where Scalar
    conforms to BinaryInteger.
    • This is useful for Tensor<Int32>(shape.dimensions).
  • Overload Tensor subscripts for Int32 and Range<Int32>.
    • This is useful for tensor[0..<x.rank].
    • I chose not to change rank and scalarCount to Int for now.

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Apr 13, 2019
@dan-zheng
Copy link
Contributor Author

This is just a start - there are definitely more usages of Int32 in Tensor APIs.
Examples include:

  • Axes (for expansion/squeezing/reduction).
  • Strides (for convolution/pooling).
  • Tensor.rank and Tensor.scalarCount.
    • Changing these to Int is consistent with TensorShape.rank and TensorShape.contiguousSize.
    • However, it breaks callers like Tensor<Int32>(rangeFrom: 0, to: rank, stride: 1).

@dan-zheng dan-zheng force-pushed the use-int-in-tf-apis branch from f971189 to 5863cbd Compare April 13, 2019 07:14
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 13, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32.
48902c2 
Friend PR: swiftlang/swift#24012
@dan-zheng dan-zheng mentioned this pull request Apr 13, 2019
Merged
rxwei
rxwei reviewed Apr 13, 2019
View reviewed changes
@rxwei
Copy link
Contributor

rxwei commented Apr 13, 2019

his is just a start - there are definitely more usages of Int32 in Tensor APIs.

I saw those places. It would be much nicer to incorporate those changes in this exact PR instead of changing them and reverting some changes made here in a separate PR.

@dan-zheng dan-zheng force-pushed the use-int-in-tf-apis branch from e29ae9e to fff5f92 Compare April 14, 2019 05:42
@dan-zheng
Copy link
Contributor Author

It would be much nicer to incorporate those changes in this exact PR instead of changing them and reverting some changes made here in a separate PR.

All user-facing APIs now use Int. Ready for re-review.

dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 14, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32.
292bdcf 
Friend PR: swiftlang/swift#24012
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng
[TF] Change APIs to use Int instead of Int32.
34ede4a 
- Let `TensorShape` store `[Int]`.
- Change all `Tensor` and `Layer` APIs to use `Int` instead of `Int32`.
- Remove unnecessary `Int` initializer calls.
@dan-zheng dan-zheng force-pushed the use-int-in-tf-apis branch from 9dcd510 to 7e6438c Compare April 15, 2019 20:03
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 15, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32.
88c71b8 
Friend PR: swiftlang/swift#24012
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 15, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32.
686570d 
Friend PR: swiftlang/swift#24012
@dan-zheng dan-zheng force-pushed the use-int-in-tf-apis branch from 7e6438c to 739e6c2 Compare April 15, 2019 20:04
rxwei
rxwei reviewed Apr 15, 2019
View reviewed changes
stdlib/public/TensorFlow/TensorShape.swift Outdated
return TensorShape(
Array(dimensions[Int(bounds.lowerBound)..<Int(bounds.upperBound)])
)
return TensorShape(Array(dimensions[bounds]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought TensorShape has a collection initializer, so i don't think Array(...) is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TensorShape does not yet have a collection initializer. I will add one.

@eaplatanios eaplatanios mentioned this pull request Apr 15, 2019
Merged
@rxwei rxwei mentioned this pull request Apr 15, 2019
Merged
@dan-zheng dan-zheng force-pushed the use-int-in-tf-apis branch from 324d1cd to 2281fbd Compare April 15, 2019 23:05
@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

rxwei
rxwei approved these changes Apr 16, 2019
View reviewed changes
@dan-zheng
Disable newly failing GPE tests.
eefcd07 
Filed TF-439 to track revisiting test failures.
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 16, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32.
af1333e 
Friend PR: swiftlang/swift#24012
dan-zheng added a commit to tensorflow/swift-apis that referenced this pull request Apr 16, 2019
@dan-zheng
[TF] Change APIs to use Int instead of Int32. (#84)
23c16ae 
Friend PR: swiftlang/swift#24012
@dan-zheng
Update checkout.
ae1f0db 
Use friend `swift-apis` PR.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

rxwei
rxwei reviewed Apr 16, 2019
View reviewed changes
stdlib/public/TensorFlow/CompilerRuntime.swift
}
return -1
}
let ranks = value.map { shape in (shape?.rank).map(Int32.init) ?? -1 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the parentheses necessary?

@rxwei rxwei merged commit e68d2fc into swiftlang:tensorflow Apr 16, 2019
@rxwei rxwei deleted the use-int-in-tf-apis branch April 16, 2019 22:39
rxwei pushed a commit to rxwei/swift that referenced this pull request May 11, 2019
@dan-zheng @rxwei
[TF] Change APIs to use Int instead of Int32. (swiftlang#24012)
8bfd4be 
- Let `TensorShape` store `[Int]`.
- Let `Tensor` subscripts take `Int` and `Range<Int>`.
- Reduce explicit calls to `Int32.init` in user code.
  - Notably, initializers taking `TensorShape` no longer need `Int32.init`.

Added two shims:
- `Tensor(_: [Int])` creates a `Tensor<Scalar>`, where `Scalar`
   conforms to `BinaryInteger`.
  - This is useful for `Tensor<Int32>(shape.dimensions)`.
- Overload `Tensor` subscripts for `Int32` and `Range<Int32>`.
  - This is useful for `tensor[0..<x.rank]`.
  - I chose not to change `rank` and `scalarCount` to `Int` for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rxwei rxwei rxwei approved these changes

@saeta saeta Awaiting requested review from saeta

@jekbradbury jekbradbury Awaiting requested review from jekbradbury

Assignees
No one assigned
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dan-zheng @rxwei