-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
GH-35623: [C++][Python] FixedShapeTensorType.ToString() should print the type's parameters #36496
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be useful! Let's add a c++ test and consider a JSON string?
be44ed5
to
a79ac96
Compare
@rok @jorisvandenbossche this should be ready for review with the suggestions included. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a suggestion for string helpers. LGTM otherwise!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for adding the utility function!
Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Thank you all for the reviews and suggestions! |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 76f987e. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 76f987e. There was 1 benchmark result indicating a performance regression:
The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…print the type's parameters (apache#36496) ### Rationale for this change The string representation of two different `FixedShapeTensorType` objects is currently the same: `extension<arrow.fixed_shape_tensor>`. ### What changes are included in this PR? Override general type `ToString()` method for `FixedShapeTensorType`. The string representation of a tensor in this PR is proposed as follows: ``` extension<arrow.fixed_shape_tensor[value_type=*, shape=[*]] ``` ### Are these changes tested? Yes, in Python and in C++. ### Are there any user-facing changes? No. * Closes: apache#35623 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
…print the type's parameters (apache#36496) ### Rationale for this change The string representation of two different `FixedShapeTensorType` objects is currently the same: `extension<arrow.fixed_shape_tensor>`. ### What changes are included in this PR? Override general type `ToString()` method for `FixedShapeTensorType`. The string representation of a tensor in this PR is proposed as follows: ``` extension<arrow.fixed_shape_tensor[value_type=*, shape=[*]] ``` ### Are these changes tested? Yes, in Python and in C++. ### Are there any user-facing changes? No. * Closes: apache#35623 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
…print the type's parameters (apache#36496) ### Rationale for this change The string representation of two different `FixedShapeTensorType` objects is currently the same: `extension<arrow.fixed_shape_tensor>`. ### What changes are included in this PR? Override general type `ToString()` method for `FixedShapeTensorType`. The string representation of a tensor in this PR is proposed as follows: ``` extension<arrow.fixed_shape_tensor[value_type=*, shape=[*]] ``` ### Are these changes tested? Yes, in Python and in C++. ### Are there any user-facing changes? No. * Closes: apache#35623 Lead-authored-by: AlenkaF <frim.alenka@gmail.com> Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: AlenkaF <frim.alenka@gmail.com>
Rationale for this change
The string representation of two different
FixedShapeTensorType
objects is currently the same:extension<arrow.fixed_shape_tensor>
.What changes are included in this PR?
Override general type
ToString()
method forFixedShapeTensorType
. The string representation of a tensor in this PR is proposed as follows:Are these changes tested?
Yes, in Python and in C++.
Are there any user-facing changes?
No.