Conversation
thomasonzhou
left a comment
thomasonzhou
left a comment
There was a problem hiding this comment.
Added some comments
thomasonzhou
left a comment
thomasonzhou
left a comment
There was a problem hiding this comment.
Added some comments on is_view_
| // Copyright (c) 2025-present WATonomous. All rights reserved. | ||
| // |
There was a problem hiding this comment.
TODO: Add test for view to ensure the invalid memory access is handled. either use a shared_ptr to manage the memory or throw an exception stating the original memory was freed
There was a problem hiding this comment.
+ other view operations if you want
| Tensor::Tensor(const std::vector<size_t> & shape, DataType dtype) | ||
| : shape_(shape) | ||
| , dtype_(dtype) | ||
| , is_view_(true) | ||
| { | ||
| calculate_strides(); | ||
|
|
||
| size_t total_elements = std::accumulate(shape_.begin(), shape_.end(), 1UL, std::multiplies<size_t>()); | ||
| byte_size_ = total_elements * get_dtype_size(dtype_); | ||
|
|
||
| allocate_memory(); | ||
| } | ||
|
|
||
| Tensor::Tensor(void * data, const std::vector<size_t> & shape, DataType dtype) | ||
| : shape_(shape) | ||
| , dtype_(dtype) | ||
| , data_(data) | ||
| , is_view_(false) | ||
| { | ||
| calculate_strides(); | ||
|
|
||
| size_t total_elements = std::accumulate(shape_.begin(), shape_.end(), 1UL, std::multiplies<size_t>()); | ||
| byte_size_ = total_elements * get_dtype_size(dtype_); | ||
| } |
There was a problem hiding this comment.
Maybe I'm misunderstanding but shouldn't this be swapped?
There is no memory ownership with the pointer so it would be a view (potentially switch to a shared_ptr to prevent access after free?)
There was a problem hiding this comment.
the tensor class actually doesn't store data inside it. It stores a pointer to the data. So what that means is that this tensor class is always a view no matter what, assuming you mean view in the literal sense that it only knows of the data's address and nothing else.
I think of view in a more high-level sense. As in if is_view_ == true then this tensor datastructure does not have the permissions to mutate the data. It can mutate the data if it isn't a view and thus the original assigned owner... does owner sound better then?
There was a problem hiding this comment.
I'm thinking tensor_view in PyTorch which shares data with the base tensor
https://docs.pytorch.org/docs/stable/tensor_view.html
Here view still allows for data mutation (which I think we should allow too), but it is not the "owner" of the data in the sense that it refers to a chunk of memory allocated when creating the original tensor.
There was a problem hiding this comment.
two things that im bending my mind around because of this:
- to what extent do we need to support various tensor operations? the intent is not to reinvent pytorch, or numpy at that. rather it should do only what we need it to do which is to be a generic type to pass tensors to downstream backends
- making this tensor class actually hold the tensor data is problematic, especially when it comes to different devices. Im currently looking into how we could "pluginize" the memory allocation so that different backends can figure out how they want to allocate the memory of the tensor
There was a problem hiding this comment.
- If we don't need to support tensor operations then it might make sense to try to create an Ort Tensor/Value directly
- I don't think it needs to hold the data, a pointer to the data is fine. In that sense maybe all of our tensors are technically views then
thomasonzhou
left a comment
thomasonzhou
left a comment
There was a problem hiding this comment.
+1
Can prune in the future if needed
* adding generic tensor type * indentation of docstring * no inline, and readme * stuff * updating package.xml * addressing batching with images * added 16 bit yuv * is view
2 participants
Adds a generic tensor type to interface between ros messages and the backend plugins.
Talks about a generic tensor type for ROS have begun https://discourse.openrobotics.org/t/native-rcl-tensor-type/49497 but the timeline for something like that is not set in stone.
For now, we'll use a generic, and relatively simple tensor type to just pass data around in Deep ROS. For the sake of replacing such a type with future types, deep_tensor should not be built on (or at least limited in expansion)