Skip to content

[android] Fix error messages; tensor creation method names with type #26219

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
Closed
IvanKobzarev wants to merge 1 commit into from
Closed

[android] Fix error messages; tensor creation method names with type #26219

IvanKobzarev wants to merge 1 commit into from

Conversation

@IvanKobzarev
Copy link
Contributor

@IvanKobzarev IvanKobzarev commented Sep 13, 2019 •
edited
Loading

After offline discussion with @dzhulgakov :

  • In future we will introduce creation of byte signed and byte unsigned dtype tensors, but java has only signed byte - we will have to add some separation for it in method names ( java types and tensor types can not be clearly mapped) => Returning type in method names

  • fixes in error messages

  • non-static method Tensor.numel()

  • Change Tensor toString() to be more consistent with python

Update on Sep 16:

Type renaming on java side to uint8, int8, int32, float32, int64, float64

public abstract class Tensor {
  public static final int DTYPE_UINT8 = 1;
  public static final int DTYPE_INT8 = 2;
  public static final int DTYPE_INT32 = 3;
  public static final int DTYPE_FLOAT32 = 4;
  public static final int DTYPE_INT64 = 5;
  public static final int DTYPE_FLOAT64 = 6;

  public static Tensor newUInt8Tensor(long[] shape, byte[] data)
  public static Tensor newInt8Tensor(long[] shape, byte[] data)
  public static Tensor newInt32Tensor(long[] shape, int[] data)
  public static Tensor newFloat32Tensor(long[] shape, float[] data)
  public static Tensor newInt64Tensor(long[] shape, long[] data)
  public static Tensor newFloat64Tensor(long[] shape, double[] data)

@pytorchbot pytorchbot added the module: android Related to Android support label Sep 13, 2019
dzhulgakov
dzhulgakov approved these changes Sep 15, 2019
View reviewed changes
Copy link
Collaborator

@dzhulgakov dzhulgakov left a comment

Choose a reason for hiding this comment

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

Thanks, more nitpicks of naming - I think it's better to follow pytorch naming convention that java's

android/pytorch_android/src/androidTest/java/org/pytorch/PytorchInstrumentedTests.java Outdated
final Module module = Module.load(assetFilePath(TEST_MODULE_ASSET_NAME));
final IValue input =
IValue.tensor(Tensor.newTensor(new long[] {1}, Tensor.allocateByteBuffer(1)));
IValue.tensor(Tensor.newByteTensor(new long[] {1}, Tensor.allocateByteBuffer(1)));
Copy link
Collaborator

@dzhulgakov dzhulgakov Sep 15, 2019

Choose a reason for hiding this comment

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

maybe use uint8 to be consistent with PyTorch's API? (torch.uint8/int8)

Copy link
Contributor Author

@IvanKobzarev IvanKobzarev Sep 16, 2019

Choose a reason for hiding this comment

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

java byte is signed,so byteBuffer for tensor content can be filled with signed bytes.

To support uint8 we need some workaround, I think with method names to force that it is unsigned.

I think that uint8 and int8 naming wil be unnatural for java world...But maybe as we expect users be familiar with pytorch it should be ok.

@dreiss , @ljk53 , what do you think about uint8, int8 naming in android java api ?

android/pytorch_android/src/androidTest/java/org/pytorch/PytorchInstrumentedTests.java Outdated
inputTensorData[i] = i;
}
final Tensor inputTensor = Tensor.newTensor(inputTensorShape, inputTensorData);
final Tensor inputTensor = Tensor.newFloatTensor(inputTensorShape, inputTensorData);
Copy link
Collaborator

@dzhulgakov dzhulgakov Sep 15, 2019

Choose a reason for hiding this comment

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

similarly - maybe Float32?

android/pytorch_android/src/main/java/org/pytorch/Tensor.java Outdated
}

public static Tensor newTensor(long[] shape, double[] data) {
public static Tensor newDoubleTensor(long[] shape, double[] data) {
Copy link
Collaborator

@dzhulgakov dzhulgakov Sep 15, 2019

Choose a reason for hiding this comment

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

Float64?

android/pytorch_android/src/main/java/org/pytorch/Tensor.java Outdated
public String toString() {
return String.format(
"Tensor_double64{shape:%s numel:%d}", Arrays.toString(shape), data.capacity());
return String.format("Tensor(%s, dtype=torch.double)", Arrays.toString(shape));
Copy link
Collaborator

@dzhulgakov dzhulgakov Sep 15, 2019

Choose a reason for hiding this comment

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

torch.double is an alias for float64:

>>> torch.double
torch.float64

@IvanKobzarev IvanKobzarev force-pushed the ik_android_api_apply_comments_2 branch 3 times, most recently from 291c54c to 724ca95 Compare September 16, 2019 20:34
@IvanKobzarev
Copy link
Contributor Author

IvanKobzarev commented Sep 16, 2019 •
edited
Loading

@dzhulgakov
I renamed types on java side to uint8, int8, int32, float32, int64, float64
(added it to PR details)

@IvanKobzarev IvanKobzarev force-pushed the ik_android_api_apply_comments_2 branch from 724ca95 to 3c6d86d Compare September 16, 2019 20:54
facebook-github-bot
facebook-github-bot reviewed Sep 16, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@IvanKobzarev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@IvanKobzarev IvanKobzarev mentioned this pull request Sep 16, 2019
Closed
facebook-github-bot
facebook-github-bot reviewed Sep 17, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@IvanKobzarev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 17, 2019

@IvanKobzarev merged this pull request in b07991f.

facebook-github-bot pushed a commit that referenced this pull request Sep 19, 2019
@IvanKobzarev @facebook-github-bot
javadocs for Tensor, IValue, Module (#26149)
436c60a 
Summary:
At the moment it includes #26219 changes. That PR is landing at the moment, afterwards this PR will contain only javadocs.

Applied all dreiss comments from previous version.
Pull Request resolved: #26149

Differential Revision: D17490720

Pulled By: IvanKobzarev

fbshipit-source-id: f340dee660d5ffe40c96b43af9312c09f85a000b
@facebook-github-bot facebook-github-bot deleted the ik_android_api_apply_comments_2 branch July 13, 2020 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@dreiss dreiss dreiss approved these changes

@dzhulgakov dzhulgakov dzhulgakov approved these changes

@ljk53 ljk53 Awaiting requested review from ljk53

Assignees

No one assigned

Labels

Merged module: android Related to Android support

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@IvanKobzarev @facebook-github-bot @dreiss @dzhulgakov @pytorchbot @mruberry