Speedup eri #127
Speedup eri #127
Conversation
|
Fails py code style. I'll fix it tomorrow.
|
|
what is the problem? do you need some help? |
|
Actually that would be nice. Do you mind rebasing the code on theochem There's also a lot of documentation that got added
|
|
I can help with that as well if needed. (I've reviewed Stijn's changes.) |
|
I'll fix this. Hang on... |
|
Just rebased and cleaned up most issues. @crisely09 : can you double check the documentation of the python and c++ code? Thanks. |
|
Looks good to me. If @crisely09 approves, it can be merged in. |
|
P.S. Once this is merged, I'll also rebase Cristina's other branch. It overlaps some with this and similar cleanups are needed. I know the drill now. :) |
|
I wonder why we didn't merge the other branch instead, the latest PR I made, it has almost the same changes, the only improvement in this one is the documentation... which I checked and it is good. What about the unit tests? Did you make different test by yourself Toon? For mine, the reference numbers were computed with the script in Mathematica I got from Julien Toulouse and Andreas Savin. |
|
Well, also we could just close the latest PR and I can make a new one with the integrals and tests that are missing, starting from the merged code... that might be easier and cleaner. What do you think? |
|
The unit tests were from you. I'll correct their origin in a extra commit. I wanted to merge this PR first because it had a more detailed commit history. Another motivation is that this PR seemed to contain older code. Merging your more recent code afterwards seemed the better option. |
|
There are still quite a bit of cleanups that need to be done. I'll take care later. |
|
One thing that remains to be done is to support Cholesky decomposition for the new types of four-center integrals. This is already documented as such but it does not work yet, nor are there tests for it. |
|
@tovrstra Yes, I can provide the reference for the Gaussian integrals. About the trivial variations, do you mean the values of the parameters used, for example, for the erfgau (Toulouse and Savin) potential?. For the ralpha-repulsion, I think I can get reference numbers for integer values of k like 0, 1 and 2, I guess, but I don't know where or how to get some for real values of k, only "by hand" for the very simple cases. |
|
@crisely09 Regarding the trivial variations: it is not completely unambiguous now what c and alpha mean. Sometimes there is a factor 1/2 in the exponent or a normalization for the Gaussian etc. A literature reference with the equations would take away any doubt. I just noticed that you had already added some tests with different powers, so don't worry about it. That is good enough. |
|
@crisely09 : Can you provide the reference please? |
|
Yes, I am sorry, I will do it tonight, I don't have my laptop with me right now. |
| /** @brief | ||
| Gaussian electron repulsion four-center integrals. | ||
|
|
||
| The potential is ??? TODO |
There was a problem hiding this comment.
The potential is c exp(-alpha r^2).
|
The oldest reference with the Gaussian potential (combined with others) seems to be that of P.M.W. Gill et al. (http://dx.doi.org/10.1016/0009-2614(96)00931-1) I'll cite that one as well. |
|
@crisely09 Can you take a look? All should be good now. |
| e^(-t)/rho^3/2 - This term helps the Taylor series to converge when t is a large | ||
| number, the factor 1/2*sqrt(rho^alpha) was "replaced" and multiplied outside, at | ||
| the end, in the laplace_of_potential function. | ||
| */ |
|
Yes, it looks good to me. |
|
Thank you for carefully reviewing! I'll fix the final things and merge after the tests have passed (unless something new comes up in the meantime obviously.) |
|
Cool, thank you Toon :) |
Fixes #48 and part of #46