GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34797

rok · 2023-03-30T11:46:30Z

Rationale for this change

We want to enable converting Tensors to FixedShapeTensorArrays and the other way around.

What changes are included in this PR?

This adds FromTensor, ToTensor to FixedShapeTensorArrays and strides method to FixedShapeTensorType.

Are these changes tested?

Yes.

Are there any user-facing changes?

This adds FromTensor, ToTensor and strides are user facing methods.

Closes: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34796

github-actions · 2023-03-30T11:46:57Z

Closes: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34796

jorisvandenbossche · 2023-04-05T07:33:51Z

In my last review of #8510 before splitting it (#8510 (review)), I still had some comments/questions on the FromTensor/ToTensor implementation. I am assuming that nothing fundamentally changed here (just split off what was at that point in that PR?).
Can you take a look at those comments and respond to them? Especially the comments about the resulting shape of FromTensor (#8510 (comment), #8510 (comment); you answered then that this was resolved because of permutation shape_, but that's no longer the case). It might be that you addressed those comments, but that's hard to see if you don't explicitly explain what you changed.

jorisvandenbossche · 2023-04-05T07:40:14Z

cpp/src/arrow/extension/fixed_shape_tensor_test.cc

+  auto cell_shapes = std::vector<std::vector<int64_t>>{{3, 4}, {3, 4}, {4, 3}, {4, 3}};
+  auto permutations = std::vector<std::vector<int64_t>>{{0, 1}, {1, 0}, {0, 1}, {1, 0}};


So I assume this is the equivalent place compared to where I commented before on the previous tests: for the second test case (shape {3, 3, 4}, cell_shape {3, 4} and permutation of {1, 0}), shouldn't the expected fixed_shape_tensor type's shape be {4, 3}?
The tensor itself has shape {3, 3, 4} (so {3, 4} for the individual tensor element), but has a permutation, so {4, 3} ?

Changed ToTensor/FromTensor to permute original shape to cell_shape and back.

I suppose we should permute also dim_names? My original thought wast to keep the original shape and only use permutation generating strides.

The spec says "dim_names ... map to the physical layout (row-major)". So they should correspond to the shape/dimensions of the extension type (and so not necessarily the original tensor).
(so I assume that is a yes)

Ok, I changed the FromTensor/ToTensor to permute the dim_names accordingly.

rok · 2023-04-06T18:48:55Z

A change introduced late in #8510 (and then removed) was that ComputeRowMajorStrides is replaced with a general ComputeStrides to calculate strides of permuted tensors which should help matching pytorch behavior:

import torch

def print_strides(shape, permutation):
    permutation2 = [p - 1 for p in permutation[1:]]
    shape2 = shape[1:]
    x = torch.randn(shape).permute(permutation)
    y = torch.randn(shape2).permute(permutation2)
    strides = [z * 8 for z in x.stride()]
    strides2 = [z * 8 for z in y.stride()]
    
    print("[full tensor] shape:", shape, "permutation:", permutation, "strides:", strides)
    print("[cell tensor] shape:", shape2, "permutation:", permutation2, "strides:", strides2)

shapes = [
    (3, 3, 4),
    (3, 3, 4),
    (3, 4, 3),
    (3, 4, 3),
]
permutations = [
    (0, 1, 2),
    (0, 2, 1),
    (0, 1, 2),
    (0, 2, 1),
]

for shape, permutation in zip(shapes, permutations):
    print_strides(shape, permutation)

[full tensor] shape: (3, 3, 4) permutation: (0, 1, 2) strides: [96, 32, 8]
[cell tensor] shape: (3, 4) permutation: [0, 1] strides: [32, 8]
[full tensor] shape: (3, 3, 4) permutation: (0, 2, 1) strides: [96, 8, 32]
[cell tensor] shape: (3, 4) permutation: [1, 0] strides: [8, 32]
[full tensor] shape: (3, 4, 3) permutation: (0, 1, 2) strides: [96, 24, 8]
[cell tensor] shape: (4, 3) permutation: [0, 1] strides: [24, 8]
[full tensor] shape: (3, 4, 3) permutation: (0, 2, 1) strides: [96, 8, 24]
[cell tensor] shape: (4, 3) permutation: [1, 0] strides: [8, 24]

jorisvandenbossche · 2023-04-07T07:54:53Z

cpp/src/arrow/extension/fixed_shape_tensor.h

+  ///
+  /// This method will create a Tensor from a FixedShapeTensorArray, setting its
+  /// first dimension as length equal to the FixedShapeTensorArray's length and the
+  /// remaining dimensions as the FixedShapeTensorType's shape.


Can you add here to the docstring that this will automatically reshape the resulting Tensor according to the permutation metadata of the FixedShapeTensorArray?

jorisvandenbossche · 2023-04-07T07:55:45Z

cpp/src/arrow/extension/fixed_shape_tensor.h

+  /// \brief Compute strides of FixedShapeTensorType
+  static Status ComputeStrides(const FixedWidthType& type,


Is there a need for having this public in addition to strides() ?

The reason it's like this now is because both FixedShapeTensorType and FixedShapeTensorArray are using ComputeStrides, but ComputeStrides is a FixedShapeTensorType method. I'll see if I can put it into another namespace.

Moved to anonymous namespace.

jorisvandenbossche

Thanks for the updates, the converted shape and dim_names look correct to me now.

ursabot · 2023-04-12T18:32:42Z

Benchmark runs are scheduled for baseline = c40e658 and contender = aff876a. aff876a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.28% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] aff876a5 ec2-t3-xlarge-us-east-2
[Failed] aff876a5 test-mac-arm
[Finished] aff876a5 ursa-i9-9960x
[Finished] aff876a5 ursa-thinkcentre-m75q
[Finished] c40e658f ec2-t3-xlarge-us-east-2
[Failed] c40e658f test-mac-arm
[Finished] c40e658f ursa-i9-9960x
[Finished] c40e658f ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

) ### Rationale for this change `std::reinterpret_pointer_cast` was introduced with FixedShapeTensor PR (#34797) but is not available on OSX (see #35143). ### What changes are included in this PR? This change switches to using `internal::checked_pointer_cast`. ### Are these changes tested? Change is tested in CI, but should also be verified on crossbow. ### Are there any user-facing changes? No. * Closes: #35143 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>

… FixedShapeTensorArray (apache#34797) ### Rationale for this change We want to enable converting Tensors to FixedShapeTensorArrays and the other way around. ### What changes are included in this PR? This adds FromTensor, ToTensor to FixedShapeTensorArrays and strides method to FixedShapeTensorType. ### Are these changes tested? Yes. ### Are there any user-facing changes? This adds FromTensor, ToTensor and strides are user facing methods. * Closes: apache#34796 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

apache#35154) ### Rationale for this change `std::reinterpret_pointer_cast` was introduced with FixedShapeTensor PR (apache#34797) but is not available on OSX (see apache#35143). ### What changes are included in this PR? This change switches to using `internal::checked_pointer_cast`. ### Are these changes tested? Change is tested in CI, but should also be verified on crossbow. ### Are there any user-facing changes? No. * Closes: apache#35143 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>

… FixedShapeTensorArray (apache#34797) ### Rationale for this change We want to enable converting Tensors to FixedShapeTensorArrays and the other way around. ### What changes are included in this PR? This adds FromTensor, ToTensor to FixedShapeTensorArrays and strides method to FixedShapeTensorType. ### Are these changes tested? Yes. ### Are there any user-facing changes? This adds FromTensor, ToTensor and strides are user facing methods. * Closes: apache#34796 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

apache#35154) ### Rationale for this change `std::reinterpret_pointer_cast` was introduced with FixedShapeTensor PR (apache#34797) but is not available on OSX (see apache#35143). ### What changes are included in this PR? This change switches to using `internal::checked_pointer_cast`. ### Are these changes tested? Change is tested in CI, but should also be verified on crossbow. ### Are there any user-facing changes? No. * Closes: apache#35143 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>

… FixedShapeTensorArray (apache#34797) ### Rationale for this change We want to enable converting Tensors to FixedShapeTensorArrays and the other way around. ### What changes are included in this PR? This adds FromTensor, ToTensor to FixedShapeTensorArrays and strides method to FixedShapeTensorType. ### Are these changes tested? Yes. ### Are there any user-facing changes? This adds FromTensor, ToTensor and strides are user facing methods. * Closes: apache#34796 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

apache#35154) ### Rationale for this change `std::reinterpret_pointer_cast` was introduced with FixedShapeTensor PR (apache#34797) but is not available on OSX (see apache#35143). ### What changes are included in this PR? This change switches to using `internal::checked_pointer_cast`. ### Are these changes tested? Change is tested in CI, but should also be verified on crossbow. ### Are there any user-facing changes? No. * Closes: apache#35143 Authored-by: Rok Mihevc <rok@mihevc.org> Signed-off-by: Dewey Dunnington <dewey@voltrondata.com>

github-actions bot added Component: C++ awaiting committer review Awaiting committer review labels Mar 30, 2023

rok mentioned this pull request Mar 30, 2023

GH-15483: [C++] Add a Fixed Shape Tensor canonical ExtensionType #8510

Merged

rok force-pushed the from_tensor_to_tensor branch 2 times, most recently from 5a62709 to f72e7a2 Compare March 30, 2023 12:07

rok changed the title ~~GH-34796: [C++] Add a Fixed Shape Tensor canonical ExtensionType~~ GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray Mar 30, 2023

rok force-pushed the from_tensor_to_tensor branch from 4bfc601 to a12a2ef Compare April 4, 2023 13:16

rok mentioned this pull request Apr 4, 2023

[R] Support for Tensor class #27499

Open

rok marked this pull request as ready for review April 4, 2023 13:27

Add FromTensor/ToTensor/strides

7279c09

rok force-pushed the from_tensor_to_tensor branch from a12a2ef to 7279c09 Compare April 4, 2023 21:19

jorisvandenbossche reviewed Apr 5, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 5, 2023

jorisvandenbossche mentioned this pull request Apr 6, 2023

GH-34882: [Python] Binding for FixedShapeTensorType #34883

Merged

Work

6d32411

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 6, 2023

rok added this to the 12.0.0 milestone Apr 6, 2023

jorisvandenbossche reviewed Apr 7, 2023

View reviewed changes

Doc change

53f695f

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 7, 2023

Permute dim_names order in FromTensor/ToTensor

0f9c33b

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 7, 2023

Moving ComputeStrides to anonymous namespace

c3e8db6

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Apr 7, 2023

rok requested a review from jorisvandenbossche April 10, 2023 22:35

jorisvandenbossche approved these changes Apr 11, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Apr 11, 2023

jorisvandenbossche merged commit aff876a into apache:main Apr 11, 2023

rok mentioned this pull request Apr 12, 2023

[Python] Bindings for FixedShapeTensorType.FromTensor/ToTensor and FixedShapeTensorArray.strides #35068

Open

rok mentioned this pull request Apr 15, 2023

GH-35143: [R][C++] Fixed shape tensor causes broken build on OSX #35154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34797

GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34797

rok commented Mar 30, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 30, 2023

jorisvandenbossche commented Apr 5, 2023

jorisvandenbossche Apr 5, 2023

rok Apr 6, 2023

rok Apr 6, 2023

jorisvandenbossche Apr 7, 2023

rok Apr 7, 2023

rok commented Apr 6, 2023

jorisvandenbossche Apr 7, 2023

rok Apr 7, 2023

jorisvandenbossche Apr 7, 2023

rok Apr 7, 2023

rok Apr 7, 2023

jorisvandenbossche left a comment

ursabot commented Apr 12, 2023

		auto cell_shapes = std::vector<std::vector<int64_t>>{{3, 4}, {3, 4}, {4, 3}, {4, 3}};
		auto permutations = std::vector<std::vector<int64_t>>{{0, 1}, {1, 0}, {0, 1}, {1, 0}};

		/// \brief Compute strides of FixedShapeTensorType
		static Status ComputeStrides(const FixedWidthType& type,

GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34797

GH-34796: [C++] Add FromTensor, ToTensor and strides methods to FixedShapeTensorArray #34797

Conversation

rok commented Mar 30, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Mar 30, 2023

jorisvandenbossche commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

ursabot commented Apr 12, 2023

rok commented Mar 30, 2023 •

edited by github-actions bot

Loading