Skip to content

[Type Refactor] Merge TType and Tensor instances as a single entity #160

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.

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

Merged
merged 3 commits into from
Dec 17, 2020

Conversation

karllessard
Copy link
Collaborator

@karllessard karllessard commented Dec 2, 2020

This PR introduces some breaking changes related to Tensor and their relationship with the NdArray data structure, in an effort of simplification and standardization.

Instances of TType (e.g. TFloat32, TInt32, etc.) now implements directly Tensor, which has been converted to a non-parameterized interface. This allows direct access to the tensor n-dimensional data space from a tensor instance, without the need to dereference to a second object by calling Tensor.data().

// Before
Tensor<TFloat32> t = TFloat32.vectorOf(0, 1, 2, 3);
TFloat32 data = t.data();
data.getFloat(1); // 1.0f

// Now
TFloat32 t = TFloat32.vectorOf(0, 1, 2, 3);
t.getFloat(1); // 1.0f

An instance of TType can now be passed directly to any method accepting a Tensor in input, e.g. Session.Runner.feed. In addition, it is now possible to cast directly a tensor of an unknown type using standard Java explicit or implicit type casting, instead of the custom method expect.

// Before
Tensor<TFloat32> t = session.run().get(0).expect(TFloat32.DTYPE);

// Now
TFloat32 = (TFloat32)s.run().get(0);

In a nutshell, important and/or breaking changes are:

  • Convert Tensor from a parameterized class to an non-parameterized interface
  • Implement directly Tensor from TType implementations
  • Add a RawTensor implementation of Tensor that takes care, among other things, of wrapping the native handle of the tensor in a protected way
  • Drop now obsolete Tensor.data(), Tensor.expect() and Operand.data() methods
  • tf.constant(TType) has to be renamed to tf.capture(TType) to avoid conflict with other Constant endpoints (left untouched).
    • In a next PR, the use of tf.capture will also become obsolete by implementing directly Operand from tensors.

CC\ @deansher , @Craigacp , @wmeddie

@karllessard karllessard requested a review from Craigacp December 8, 2020 18:44
Copy link
Collaborator

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

While we're in here, is it worth moving the contents of the types.family package into the types package? It seems strange to me to have the interfaces at a lower level than the concrete implementations.

* @param tensor a Tensor holding the constant value
* @return a constant of the same data type as `tensor`
*/
public <T extends TType> Constant<T> capture(T tensor) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/capture/copy/ or immutableCopy ? I don't particularly like the name capture here as it doesn't actually change the state of the tensor argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like capture neither. As discussed here, eventually the tensors could be automatically converted to constants so we will not need to call this endpoint anymore. Still, I now think copy do sound more appropriate as well, I'll make the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok as copy already exist as another primitive op, I've decided to rename capture by constantOf, as it is close enough to the original constant name and will be easier to find by our users on auto-completion.

@karllessard
Copy link
Collaborator Author

While we're in here, is it worth moving the contents of the types.family package into the types package? It seems strange to me to have the interfaces at a lower level than the concrete implementations.

I'm not opposed to this but let's keep it for another PR, more changes in this package are planned for the refactoring in progress.

@karllessard karllessard force-pushed the type-refactor-tensorinf branch from 72d0653 to 598fdc3 Compare December 13, 2020 04:03
@karllessard karllessard force-pushed the type-refactor-tensorinf branch from 598fdc3 to 8773ccc Compare December 13, 2020 21:00
Copy link
Collaborator

@Craigacp Craigacp left a comment

Choose a reason for hiding this comment

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

LGTM

@karllessard karllessard merged commit 98979f0 into tensorflow:master Dec 17, 2020
rnett pushed a commit to rnett/tensorflow-java that referenced this pull request Dec 28, 2020
…ensorflow#160)

* Merge TType and Tensor instances as a single entity

* Rectify documentation based on PR review

* Rebase on master
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