Skip to content

Conversation

@ksasi
Copy link

@ksasi ksasi commented Apr 24, 2019

Added description property to TensorShape by conforming to CustomStringConvertible so that we can obtain the desired TensorShape printing.

Resolves TF-447.

@ksasi ksasi marked this pull request as ready for review April 24, 2019 20:06
@dan-zheng dan-zheng requested review from dan-zheng and rxwei April 24, 2019 20:10
Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Could you please add a test to test/TensorFlowRuntime/tensor.swift?

Something like:

TensorTests.testAllBackends("TensorShapeDescription") {
  expectEqual("TensorShape([2, 2])", Tensor<Int32>(ones: [2, 2]).shape.description)
  expectEqual("TensorShape([])", Tensor(1).shape.description)
}

@inlinable
public var description: String {
return "TensorShape(\(dimensions))"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix indentation:

Suggested change
}
}

@dan-zheng
Copy link
Contributor

dan-zheng commented Apr 24, 2019

A thought: how about using just [2, 2] instead of TensorShape([2, 2])?
cc @rxwei

@rxwei
Copy link
Contributor

rxwei commented Apr 24, 2019

I agree that just [2, 2] is better.

}

extension TensorShape : CustomStringConvertible {
@inlinable
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for @inlinable here.

extension TensorShape : CustomStringConvertible {
@inlinable
public var description: String {
return "TensorShape(\(dimensions))"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we think that [2, 2] is more ideal, calling dimensions.description should be sufficient.

Suggested change
return "TensorShape(\(dimensions))"
return dimensions.description

Added test to test/TensorFlowRuntime/tensor.swift
@rxwei
Copy link
Contributor

rxwei commented Apr 25, 2019

@swift-ci please test tensorflow

1 similar comment
@rxwei
Copy link
Contributor

rxwei commented Apr 25, 2019

@swift-ci please test tensorflow

@ksasi
Copy link
Author

ksasi commented Apr 25, 2019

Updated changes as suggested i.e.
1.Fixed indentation
2.Removed @inlinable
3.Updated "TensorShape((dimensions))" to dimensions.description

Also added test to test/TensorFlowRuntime/tensor.swift as below:

TensorTests.testAllBackends("TensorShapeDescription") {
  expectEqual("[2, 2]", Tensor<Int32>(ones: [2, 2]).shape.description)
  expectEqual("[]", Tensor(1).shape.description)
}

@rxwei
Copy link
Contributor

rxwei commented Apr 25, 2019

Thanks!

Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Thank you!

@dan-zheng dan-zheng merged commit 0c45261 into swiftlang:tensorflow Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants