Skip to content
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

[BUG] GPU degreees function does not overflow #109

Closed
revans2 opened this issue Jun 3, 2020 · 2 comments · Fixed by #5996
Closed

[BUG] GPU degreees function does not overflow #109

revans2 opened this issue Jun 3, 2020 · 2 comments · Fixed by #5996
Assignees
Labels
bug Something isn't working good first issue Good for newcomers SQL part of the SQL/Dataframe plugin

Comments

@revans2
Copy link
Collaborator

revans2 commented Jun 3, 2020

Describe the bug
On very large numbers GpuToDegrees will not overflow and produce inf when the GPU version does. This appears to be an order of operations issues where the CPU version multiplies by 180 and then divides by pi, while we for performance reasons multiply by (108/pi). It should be a minor fix to make this work.

A python test is being added to reproduce this issue.

@revans2 revans2 added bug Something isn't working SQL part of the SQL/Dataframe plugin good first issue Good for newcomers labels Jun 3, 2020
@thirtiseven
Copy link
Collaborator

thirtiseven commented Jul 15, 2022

The toDegrees in Spark is a direct call to the toDegrees in the java math library, but since JDK 9 the java implementation has changed to the same order of calculation as the current spark-rapids (from angrad * 180.0 / PI to angrad * (180.0 / PI)), So the results of spark will differ depending on the JDK version in the runtime environment.

Do we need to add support for higher versions of the JDK to ToDegrees to match spark's output? It can perhaps be implemented by determining the Java version when executing the function.

Some tests: current code is XFAIL on JDK8 and XPASS on JDK11, and following code in this PR is PASS on JDK8 and FAIL on JDK11 for case test_degrees.

case class GpuToDegrees(child: Expression) extends GpuUnaryMathExpression("DEGREES") {

  override def doColumnar(input: GpuColumnVector): ColumnVector = {
    val tmp = withResource(Scalar.fromDouble(180d)) { multiplier =>
      input.getBase.mul(multiplier)
    }
    withResource(tmp) { _ =>
      withResource(Scalar.fromDouble(Math.PI)) { pi =>
        tmp.div(pi)
      }
    }
  }
}

@revans2
Copy link
Collaborator Author

revans2 commented Jul 15, 2022

If the JDK itself decided that our implementation is better, then I am inclined to just keep the current one and add docs explaining the difference. Then we can have tests for an exact answer that are skipped if the JDK version is JDK 8 or below.

@sameerz do you have any objection to this?

tgravescs pushed a commit to tgravescs/spark-rapids that referenced this issue Nov 30, 2023
Signed-off-by: spark-rapids automation <70000568+nvauto@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers SQL part of the SQL/Dataframe plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants