Skip to content

Conversation

@edponce
Copy link
Contributor

@edponce edponce commented Sep 9, 2021

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

github-actions bot commented Sep 9, 2021

@github-actions
Copy link

github-actions bot commented Sep 9, 2021

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

@edponce
Copy link
Contributor Author

edponce commented Sep 9, 2021

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?

@edponce edponce changed the title ARROW-13937: [C++][Compute] Add explicit output values to sign function for unsigned inputs ARROW-13937: [C++][Compute] Add explicit output values to sign function and fix unary type checks Sep 13, 2021
@edponce edponce force-pushed the ARROW-13937-Compute-Add-explicit-output-values-to-si branch from 1fb5c27 to 459e926 Compare September 13, 2021 05:34
@edponce
Copy link
Contributor Author

edponce commented Sep 13, 2021

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

@lidavidm
Copy link
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.

Copy link
Member

@lidavidm lidavidm left a comment

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.

@lidavidm lidavidm closed this in 9122149 Sep 13, 2021
@edponce
Copy link
Contributor Author

edponce commented Sep 13, 2021

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).

Closes apache#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 free to 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