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.

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

Shapes expect i64 instead of usize #51

Open
grtlr opened this issue Jun 7, 2019 · 4 comments
Open

Shapes expect i64 instead of usize #51

grtlr opened this issue Jun 7, 2019 · 4 comments

Comments

@grtlr
Copy link
Contributor

grtlr commented Jun 7, 2019

Maybe this comes from generating the bindings automatically, but is there a use case for expecting &[i64] in Tensor::reshape and similar methods? Maybe it would make sense to change this to &[usize]? This change would also bring Tensor more in line with ndarray, which uses usize to represent shapes.

@LaurentMazare
Copy link
Owner

Right, having usize at some places would be better. That said, a commonly used value is -1 both for indexes and shapes.
For indexes it means start from the end, -1 would be the last value, -2 the value before and so on.
For shapes it's used for a single dimension and gets an appropriate value based on the tensor total number of elements (this number of element has to be divisible by the product of sizes for other dimensions of course). E.g. one could write a.reshape([batch_size, -1]) to flatten a.
This explains why i64 are used there, it's less verbose than using Option<usize> for shapes or some other alternative for indexes, it's also close to how the python api works but it certainly doesn't feel very rust idiomatic.

@grtlr
Copy link
Contributor Author

grtlr commented Jun 7, 2019

Thanks for the quick reply! I like the idea of tch-rs staying close to the Python API. For now I will just start to write some more idiomatic Rust wrappers around some of these methods for my own needs. Maybe in the future we can have a separate crate which handles these kind of things. Such a crate was briefly discussed in #12, do you have any ideas in mind here?

@LaurentMazare
Copy link
Owner

I don't have much concrete ideas but maybe a couple of these would be good:

  • The tensor type could be parameterized by the kind of element embedded would be nice, e.g. Tensor<i64>.
  • The tensor type could embed the number of dimensions, e.g. Tensor<d2, i64> or Tensor2d<i64>.
  • Maybe tensors with named dimensions although that may be quite a stretch.

I would have hoped for the first two points to be doable when generating the binding code but I haven't found enough information in the Declarations.yaml file that is used for generation. Fwiw tensorflow provides some description of operations with all the details on the supported types which make it possible to generate better type annotations, e.g. for conv2d here is some generated ocaml code: conv2d can only apply to tensors of float or double and return a tensor of the same type.

Also if you get some interesting wrappers that you think should be part of tch-rs let me know via a PR or an issue.

@LaurentMazare
Copy link
Owner

Thinking more about this, it would certainly be nice to use more of usize for dimensions/shape, e.g. for size or for the various nn creators. The only exception I can think of are view, get, narrow which should handle negative indexing and values. I'll try making some changes for this when I get a chance.

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

No branches or pull requests

2 participants