Add a view and flat method to TypedNeuropodTensor
#441
Conversation
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
==========================================
- Coverage 88.23% 88.21% -0.02%
==========================================
Files 101 101
Lines 6356 6409 +53
==========================================
+ Hits 5608 5654 +46
- Misses 748 755 +7
Continue to review full report at Codecov.
|
| @@ -460,7 +514,9 @@ public: | |||
| static int cmp(const std::string &lhs, const std::string &rhs) { return lhs.compare(rhs); } | |||
|
|
|||
| // Based on https://en.cppreference.com/w/cpp/language/operators | |||
| // TODO(vip): Simplify these operators | |||
There was a problem hiding this comment.
| auto new_num_elements = compute_num_elements(requested_dims); | ||
| auto num_elements = get_num_elements(); | ||
|
|
||
| if (new_num_elements != num_elements) |
There was a problem hiding this comment.
If we so strictly check it, we do need requested_dims at all?
Do we expect that requested_dims can be ever different than Tensor's dims?
There was a problem hiding this comment.
We're checking that the number of elements match, not that the dims match.
In fact, we expect that requested_dims will almost always be different than the tensor's dims.
The purpose of view is to view a tensor as if it had a different set of dimensions. The major restriction here is that the total number of elements must be the same.
For example, viewing a (10, 10, 3) tensor as a (10, 30) or (1, 300) or (300,) tensor is fine because the total number of elements is the same.
view is quite similar to reshape in numpy except it guarantees that a copy of the underlying data is not made (see https://numpy.org/doc/stable/reference/generated/numpy.reshape.html for more details). Also see the link to view in pytorch in the PR description
There was a problem hiding this comment.
I see, I expected that having required_dims should allow us to have view even if we have "smaller" window but I guess requirement for same "size" is fine.
VivekPanyam
force-pushed
the
view
branch
2 times, most recently
from
a9fa234 to
1736def
Compare
| } | ||
|
|
||
| std::vector<int32_t> expected(15); | ||
| std::iota(expected.begin(), expected.end(), 0); |
There was a problem hiding this comment.
This is cool, first time I see it, somehow missed this feature of C++11.
btw, Go has "iota" for emum/const initialization https://yourbasic.org/golang/iota/
| size_t compute_num_elements(const std::vector<int64_t> &dims) | ||
| { | ||
| // Get the number of elements in the tensor by multiplying all the dims together | ||
| return std::accumulate(dims.begin(), dims.end(), static_cast<size_t>(1), std::multiplies<>()); |
There was a problem hiding this comment.
Not critical, I left comments in prev iteration, exact headers for these algos are:
#include <functional> // std:: multiplies
#include <numeric> // std::accumulate
There was a problem hiding this comment.
Ah I forgot to submit my response to your comments. Based on include-what-you-use from Google:
"Include what you use" means this: for every symbol (type, function variable, or macro) that you use in foo.cc, either foo.cc or foo.h should #include a .h file that exports the declaration of that symbol.
For std::accumulate, my updates did add #include <algorithm> to the .cc file
For std::multiplies, neuropod_tensor.hh includes <functional>
There was a problem hiding this comment.
Note: after looking just now, the include-what-you-use definition contradicts the Google style guide recommendation:
... foo.cc should include bar.h if it uses a symbol from it even if foo.h includes bar.h. ...
That said, I think it's okay for us to stick to the definition in my comment above. If we introduce IWYU into lint/CI, we can enforce it throughout the codebase.
There was a problem hiding this comment.
Tracked by #446
There was a problem hiding this comment.
Just found that github formatter hided header name because of not escaped <. Just minor note:
https://en.cppreference.com/w/cpp/algorithm/accumulate "Defined in header " , I guess that "algorithm" includes it as well and this is why it works and is under the same "include-what-you-use" topic you mentioned.
Thanks for creating Issue.
Summary:
This PR adds a
viewmethod toTypedNeuropodTensor<T>that allows users to view and access a tensor as if it had a different set of dimensions.Somewhat similar to https://pytorch.org/docs/stable/tensors.html#torch.Tensor.view except we currently don't support
-1dimensions. Our implementation is also simpler because allNeuropodTensors are currently contiguous.The
viewmethod returns aTensorViewwhich is a thin wrapper aroundTensorAccessorto store strides and dimensions.Changes made in the returned view modify the original tensor data.
flatis a simple method that returns a 1D view of the tensor (effectivelyreturn view(get_num_elements())). See https://www.tensorflow.org/api_docs/cc/class/tensorflow/tensor#flat for more details.Test Plan:
CI + added tests