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

[WIP] Soa spherical tensor #4714

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

anbenali
Copy link
Contributor

@anbenali anbenali commented Aug 25, 2023

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

Explicitly unrolling the functions in the Spherical harmonics evaluations based on Sloan, P., Efficient Spherical Harmonic Evaluation, Journal of Computer Graphics Techniques, vol. 2, no. 2, 84-90, 2013. Gets 1.6 to 6X speedup depending on the angular momentum.

Total Gain for Fe(CO)6 using bfd_vtz :1.8X

Total Gain for Au single Atom using ccecp_aug-cc-pcv5z : 2.4 X

Total Gain for H2O using ccecp_cc-pcvtz: 1.4X

Tests conducted on laptop.

What type(s) of changes does this code introduce?

Delete the items that do not apply

  • Refactoring (no functional changes, no api changes)

Does this introduce a breaking change?

  • no

What systems has this change been tested on?

  • Personal Laptops
  • Cooley [ALCF]

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes. This PR is up to date with current the current state of 'develop'
  • Yes. Code added or changed in the PR has been clang-formatted
  • No. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • Yes/No. Documentation has been added (if appropriate)

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

  • Please post some of the data you have on the performance improvements with details of the system (basis etc.) and hardware. I do believe the result -- it is more than unrolling.
  • Is this code a cut & paste from the cited article or do you have a generator code? If the latter we should capture it, just as we have been capturing Mark's python codes for optimization and wavefunction testing.
  • I hope we can get test coverage near 100% for these foundational numeric functions.

@prckent
Copy link
Contributor

prckent commented Aug 25, 2023

Test this please

@ye-luo
Copy link
Contributor

ye-luo commented Sep 5, 2023

leave some idea. We can technically generate constants using constexpr functions to avoid storing them in the source.

#include <cmath>
#include <iostream>

constexpr double foo(double a)
{
  double res = 0;
  for(int i=0;i<2;i++)
    res+= ::sin(a+i);
  return res;
}

int main()
{
  constexpr double b = foo(2); # can be evaluated at compile time even if it involves math function calls in it.
  std::cout << "b = " <<b <<std::endl;
  return b;
}

@anbenali
Copy link
Contributor Author

anbenali commented Sep 7, 2023

  • Please post some of the data you have on the performance improvements with details of the system (basis etc.) and hardware. I do believe the result -- it is more than unrolling.

I added the data. Should I create tests for these or just the numbers as is are enough?

* Is this code a cut & paste from the cited article or do you have a generator code? If the latter we should capture it, just as we have been capturing Mark's python codes for optimization and wavefunction testing.

Yes There is a code generator. Kevin (@kgasperich ) will supply it.

* I hope we can get test coverage near 100% for these foundational numeric functions.

There is an existing test already covering this function but it is not clear if it is being called. Once the deadlines are past we will "clean" it.

@prckent
Copy link
Contributor

prckent commented Sep 7, 2023

Generator code should be added in case "we" end up redoing this.

@prckent
Copy link
Contributor

prckent commented May 21, 2024

Ping. What is the status/plan for this code?

To be merged this will need at least the generator code added, as previously requested, and -- similar to how Mark Dewing did this for other generated code -- suitably clear comment guards around the generated code. ( "DO NOT EDIT AUTOMATICALLY GENERATED BY XYZ.PY" ). Automagically generated unit tests would also be ideal -- we ought to be able to get ~100% coverage for these functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants