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

Improve basis function implementation #1338

Merged
merged 4 commits into from Jun 27, 2022
Merged

Conversation

davidscn
Copy link
Member

@davidscn davidscn commented Jun 23, 2022

Main changes of this PR

Switches the basis function implementation from using std::pow to a pow variant relying on recursion.

Motivation and additional information

The main problem is that std::pow operates on a double exponent (there is no int variant), but we use it in the basis function implementation only for integer exponents. The basis function evaluation routine is heavily performance critical. With the changes here, we get a considerable performance gain in both RBF implementations (Eigen and PETSc). Here some timings for the (expensive) computation of matrix A as well as the overall mapping computation:

result-asmA.pdf

result-computet.pdf

Example: For the c6 Wendland RBF, we see a speed-up of ~4.

Also related to #1320

Author's checklist

  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I ran make format to ensure everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.16.3.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?

@davidscn davidscn added the enhancement A new feature, a new functionality of preCICE (from user perspective) label Jun 23, 2022
@davidscn davidscn requested a review from fsimonis June 23, 2022 09:46
using std::log;
using std::pow;
return 1.0 - 30.0 * pow(p, 2.0) - 10.0 * pow(p, 3.0) + 45.0 * pow(p, 4.0) - 6.0 * pow(p, 5.0) - 60.0 * log(pow(p, pow(p, 3.0)));
return 1.0 - 30.0 * math::pow_int<2>(p) - 10.0 * math::pow_int<3>(p) + 45.0 * math::pow_int<4>(p) - 6.0 * math::pow_int<5>(p) - math::pow_int<3>(p) * 60.0 * std::log(std::max(p, 1e-15));
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of going for this overly complicated formula in the end log(p^(p^3)) I selected now a fixed cutoff for the log (1e-14). The overall product will be zero anyway, as well multiply it by (1e-14)^3.

@fsimonis
Copy link
Member

It is obscure, but there actually is an overload that matches a double base and a integral exponent. Its overload 7 of the docs.

So, the exponent should be written 2 instead of 2.0.

@davidscn
Copy link
Member Author

It is obscure, but there actually is an overload that matches a double base and a integral exponent. Its overload 7 of the docs.

I was indeed a bit puzzled about this version. The docs say "If any argument has integral type, it is cast to double."

So, I guess it is still double?

Anyway, the recursion version is still faster than the std::pow(arg, int(N) method:

result-asmA.pdf

@fsimonis
Copy link
Member

fsimonis commented Jun 23, 2022

Ah I see. You lifted the exponent into the type system. Makes sense that it is faster.

You did test using a Release build, right?

@davidscn
Copy link
Member Author

Ah I see. You lifted the exponent into the type system. Makes sense that it is faster.

Yes (although I would have expected a somewhat similar result for small exponents from the STL).

You did test using a Release build, right?

Yes.

src/math/math.hpp Outdated Show resolved Hide resolved
src/math/math.hpp Outdated Show resolved Hide resolved
src/math/math.hpp Show resolved Hide resolved
@davidscn davidscn merged commit 42546e7 into precice:develop Jun 27, 2022
@davidscn davidscn deleted the recursion branch June 27, 2022 06:03
davidscn added a commit to davidscn/precice that referenced this pull request Jun 30, 2022
* Implement basis functions via recursion
* Add unit test for pow method
* Use math::NUMERICAL_ZERO_DIFFERENCE for tolerance
* Apply suggestions from code review and add changelog entry
@davidscn davidscn added this to the Version 2.5.0 milestone Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature, a new functionality of preCICE (from user perspective)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants