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

Remove constexpr square root call in CDI #1069

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

alexrlongne
Copy link
Contributor

@alexrlongne alexrlongne commented Jun 10, 2021

Background

  • I used constexpr square root in a Require function to make sure overflow would not result when an input value was squared. I thought this was a good idea because the sqrt(std::numeric_limits<double>::max) was previously being computed every time through this loop. Unfortunately, it seems that debug code versions were not evaluating this expression at compile time and it was causing very slow runtimes and even more problems under profiling (the ce_sqrt is not an efficient version of sqrt it's just one that can be marked constexpr!). This PR removes the ce_sqrt call and replaces it with the sqrt of the max double. There is a comment explaining the magic number.

Purpose of Pull Request

  • Fix slow runtimes and profiling times in debug mode

Description of changes

  • Replace constexpr squar root call with a numeric value

Status

@KineticTheory
Copy link
Collaborator

Something seems wrong with the history of this branch. The diff shows a bunch of files from kde as modified. Can you rebase and/or squash?

+ Change Require to use numeric value instead of ce_sqrt value, which
was slow in some circumstances
@alexrlongne
Copy link
Contributor Author

@KineticTheory Sorry about that, it should be fixed now.

@KineticTheory KineticTheory added this to the Draco-7_11_0 milestone Jun 10, 2021
@KineticTheory KineticTheory self-requested a review June 10, 2021 21:53
@KineticTheory KineticTheory merged commit eec7c81 into lanl:develop Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants