LaurentMazare / tch-rs

Rust bindings for the C++ api of PyTorch.
Apache License 2.0
4.28k stars 340 forks source link

Discussion on Tensor type conversions #281

Open jerry73204 opened 3 years ago

jerry73204 commented 3 years ago

It is due to my closed #198. It is outdated for a while and I think it should be discarded too. Nevertheless, let me leave some concerns about type conversions in current implementation. Some changes would be made as soon as some concerns become clear.

edlanglois commented 3 years ago

I don't think that Copy can be implemented for Tensor because Copy is for "types whose values can be duplicated simply by copying bits". If you did that with Tensor you'd end up with both pointing to the same object on the C++ side which would result an extra refcount subraction when the two tch Tensors go out of scope.

jerry73204 commented 3 years ago

@edlanglois I agree with your points. The cloning/coping of Tensor is correct only when the refcount is increased implicitly, if it is a shallow copy.

Another concern is that whether .clone() is shallow or deep affects the semantics of ownership. Currently, the self-modifying methods like .absolute_(&mut self) expects unique ownership. The shallow copy shares the underling data, making the ownership unsound. If libtorch knows concurrent self-modifying operations, just like many other concurrent data structures like dashmap, it should expect shared ownership.

EDIT: Let's move the discussion to #370.

jerry73204 commented 3 years ago

More thoughts about this thread.