Skip to content

ARROW-13937: [C++][Compute] Add explicit output values to sign function and fix unary type checks - #11113

Closed
edponce wants to merge 2 commits into
apache:masterfrom
edponce:ARROW-13937-Compute-Add-explicit-output-values-to-si
Closed

ARROW-13937: [C++][Compute] Add explicit output values to sign function and fix unary type checks#11113
edponce wants to merge 2 commits into
apache:masterfrom
edponce:ARROW-13937-Compute-Add-explicit-output-values-to-si

Conversation

@edponce

@edponceedponce commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

This PR adds explicit output values to the sign function for the unsigned integer kernel. This makes the kernel more readable and consistent with the other sign kernels.

Also, enable_if checks are fixed for scalar unary arithmetic functions (sign, negate, absolute, ceil/floor/trunc).

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has no components in JIRA, make sure you assign one.

@edponce

Copy link
Copy Markdown
ContributorAuthor

After revisiting the sign compute function, I noticed that kernels are registered with Int8Type for integral types and corresponding floating-point for floating-point types. The kernels' Call methods are stated to be enabled if they return unsigned int, signed int, and floating-point. The unsigned int kernel is therefore never invoked. To fix this, the enable_if checks should be done against Arg0 or Arg1.

Also, shouldn't the floating-point kernels also return the same int type for sign function?

@edponceedponce changed the title ARROW-13937: [C++][Compute] Add explicit output values to sign function for unsigned inputsARROW-13937: [C++][Compute] Add explicit output values to sign function and fix unary type checksSep 13, 2021
@edponce
edponceforce-pushed the ARROW-13937-Compute-Add-explicit-output-values-to-si branch from 1fb5c27 to 459e926CompareSeptember 13, 2021 05:34
@edponce

Copy link
Copy Markdown
ContributorAuthor

@lidavidm Please help with reviews when you get a chance.

@lidavidm

Copy link
Copy Markdown
Member

Also, shouldn't the floating-point kernels also return the same int type for sign function?

np.sign returns the same type as the input.

@lidavidmlidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Having test coverage info would help us realize that overloads/branches aren't being taken, I think.

@edponce

Copy link
Copy Markdown
ContributorAuthor

cc @lidavidm The floating-point kernels need to return a floating-point type because of sign(NaN) = NaN and NaN is not support in integer types. For the integral kernels, Int8 is returned because output can be any of (-1,0,1).

I was thinking last week of tests for code coverage. I actually know of several snippets of dead code.

ViniciusSouzaRoque pushed a commit to s1mbi0se/arrow that referenced this pull request Oct 20, 2021
…on and fix unary type checks
This PR adds explicit output values to the sign function for the unsigned integer kernel. This makes the kernel more readable and consistent with the other sign kernels.
Also, enable_if checks are fixed for scalar unary arithmetic functions (sign, negate, absolute, ceil/floor/trunc).
Closesapache#11113 from edponce/ARROW-13937-Compute-Add-explicit-output-values-to-si
Authored-by: Eduardo Ponce <edponce00@gmail.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edponce@lidavidm