Skip to content

Conversation

@wavefunction91
Copy link
Owner

Unify implementation of radial quadrature generation. Closes #12.

@wavefunction91
Copy link
Owner Author

@susilehtola This should add everything that we need to unify the implementations. Dox incoming.

I've written the traits for TA, but I can't consolidate the Chebyshev roots with the closed form expression given in https://doi.org/10.1002/jcc.10211 (which also agrees with the Psi4 implementation as far as I can tell). For order n Chebyshev quadrature, the points are sinusoidally generated with spacing pi/n, where as these implementations generate on pi/(n+1). Unclear how to resolve this. Is there a reason that the TA radial qudature of order n uses the Chebyshev nodes of order n+1? We just need to be able form the logic to relate the dimension of the base quadrature with that of the transformed quadrature.

@susilehtola
Copy link
Collaborator

I've written the traits for TA, but I can't consolidate the Chebyshev roots with the closed form expression given in https://doi.org/10.1002/jcc.10211 (which also agrees with the Psi4 implementation as far as I can tell). For order n Chebyshev quadrature, the points are sinusoidally generated with spacing pi/n, where as these implementations generate on pi/(n+1). Unclear how to resolve this. Is there a reason that the TA radial qudature of order n uses the Chebyshev nodes of order n+1? We just need to be able form the logic to relate the dimension of the base quadrature with that of the transformed quadrature.

I can't say I fully understand what you are saying, but yes, it looks like there may be an issue with the Psi4 code. I've opened up a ticket psi4/psi4#3004

@susilehtola
Copy link
Collaborator

Treutler and Ahlrichs clearly state that

These mappings have been combined with Chebyshev quadratures of the first (T1) and second (T2) kind, where $-1 \le x \le 1$, and a third formula based on the weight function $\sqrt{x/(1-x)}, 0 \le x \le 1$, here denoted T3, which is a variant of T1 (for formulas of T1, T2, and T3, see p. 889 of Ref. 1).

Their Ref. 1 is Abramowicz and Stegun, and page 889 gives the following rules:

  1. Chebyshev polynomials of the first kind

$$ \int_{-1}^{1} \frac {f(x)} {\sqrt{1-x^2}} {\rm d}x = \sum_{i=1}^{n} w_i f(x_i) + R_{n} $$

with abscissas and weights

$$x_i = \cos \frac {(2i-1) \pi} {2n} $$

$$w_i = \frac {\pi} {n}$$

  1. Chebyshev polynomials of the second kind

$$ \int_{-1}^{1} f(x) \sqrt{1-x^2} {\rm d}x = \sum_{i=1}^{n} w_i f(x_i) + R_{n} $$

with abscissas and weights

$$x_i = \cos \frac {i \pi} {n+1} $$

$$w_i = \frac {\pi} {n+1} \sin^2 \frac {i \pi} {n+1}$$

  1. The third rule is

$$ \int_{0}^{1} f(x) \sqrt{\frac {x} {1-x}} {\rm d}x = \sum_{i=1}^{n} w_i f(x_i) + R_{n} $$

with abscissas and weights

$$x_i = \cos^2 \frac {(2i-1) \pi} {2(2n+1)} $$

$$w_i = \frac {2 \pi} {2n+1} x_i$$

The rules T1 and T2 are in perfect agreement with the current equations in https://github.com/wavefunction91/IntegratorXX/blob/master/include/integratorxx/quadratures/gausscheby1.hpp and https://github.com/wavefunction91/IntegratorXX/blob/master/include/integratorxx/quadratures/gausscheby2.hpp.

As we discuss in J. Chem. Phys. 157, 174114 (2022), Gill and Chien's equations are wrong for Mura-Knowles quadrature (they write it with trapezoidal quadrature instead of Chebychev quadrature), so I would not rely on their equations being correct for Treutler-Ahlrichs, either...

@wavefunction91
Copy link
Owner Author

Thanks for looking in to it - glad to know it's not just something I'm missing in my understanding.

Gill and Chien's equations are wrong for Mura-Knowles quadrature (they write it with trapezoidal quadrature instead of Chebychev quadrature)

That's what we do too... Interesting, @hjjvandam what does NWChem do? I was able to get (within epsilon) identical grids to NWChem at one point. If there's just an honest discrepancy in the lit / community software, we can expose both via the new modular API.

@loriab
Copy link

loriab commented Jul 7, 2023

I looked at Susi's equations from Abramowicz a little closer, and the below is what I would do for psi4. The Type1 call is new and the Type2 call isn't actually changing code. I think the scaling for unit form actually is present, but it gets folded into the weight. Of course that doesn't explain why IntegratorXX and Psi4 are producing different values. Could it be something about Integrator looping from i=0 and filling arrays weights[i] with i+1 and Psi looping from i=1 and filling arrays weights[i-1] with i?

Basically, I don't have an answer, so putting forward status from another set of eyes.

void RadialGridMgr::getChebychevRootsType1(int n, double r[], double w[]) {
    double piOver2N = M_PI / (2 * n);
    for (int i = 1; i <= n; i++) {
        double x = cos((2 * i - 1.0) * piOver2N);
        r[i - 1] = x;
        w[i - 1] =
            2 * piOver2N * sqrt(1.0 - x * x);  
            // raw weight = pi / n = 2 * piOver2N
            // but scaling for unit form by *= sqrt(1 - x^2), final weight = 2 * piOver2N * sqrt(1 - x^2) = 2 * piOver2N * sin((2 * i - 1.0) * piOver2N)
    }
}

void RadialGridMgr::getChebychevRootsType2(int n, double r[], double w[]) {
    double piOverNPlusOne = M_PI / (n + 1);
    for (int i = 1; i <= n; i++) {
        double x = cos(i * piOverNPlusOne);
        r[i - 1] = x;
        w[i - 1] =
            piOverNPlusOne * sqrt(1.0 - x * x);  
            // raw weight = piOverNPlusOne * sin^2 (i * piOverNPlusOne) = piOverNPlusOne * (1 - cos^2 (i * piOverNPlusOne)) = piOverNPlusOne * (1 - x^2)
            // but scaling for unit form by /= sqrt(1 - x^2), final weight = piOverNplusOne * sqrt(1 - x^2) = piOverNPlusOne * sin(i * piOverNPlusOne)
    }                        

@wavefunction91
Copy link
Owner Author

There were bugs in the GC quadratures, fixed in #32 (+ added unit tests). These were place holders before we started the refactor, now they've been validated.

so putting forward status from another set of eyes.

Those seem to agree with what's currently getting PR-ed (modulo trig optimizations). I think this makes things consistent between the two codes for the base quadratures, would be easy enough to check.

The remaining question is how to validated that the generated Treutler-Ahlrichs (TA) quadratures are consistent. As @susilehtola pointed out, the Gill paper might have the wrong final expressions, so I may have been chasing my tail a bit trying to get things to agree. Do we have a third party source to validate the final TA quadratures?

@loriab
Copy link

loriab commented Jul 8, 2023

Ok, so Psi4 is believed correct at the moment? Whew, but let me know if that changes. I don't have any particular sources on the TA quadratures.

@wavefunction91
Copy link
Owner Author

If the quadrature generated on this line is meant to be Chebyshev of the second kind, then yes, this is correct.

@loriab
Copy link

loriab commented Jul 8, 2023

If the quadrature generated on this line is meant to be Chebyshev of the second kind, then yes, this is correct.

Yes, that line is second kind -- I'll fix up the naming and documentation.

@susilehtola
Copy link
Collaborator

susilehtola commented Jul 8, 2023

The remaining question is how to validated that the generated Treutler-Ahlrichs (TA) quadratures are consistent. As @susilehtola pointed out, the Gill paper might have the wrong final expressions, so I may have been chasing my tail a bit trying to get things to agree. Do we have a third party source to validate the final TA quadratures?

I can get in touch with the Turbomole developers, since the original implementation of Treutler and Ahlrichs is in that code... but it does now look like the expressions are correct.

@wavefunction91
Copy link
Owner Author

@susilehtola I was able to confirm that generating the quadratures via GC-2 -> TA-M4 gave the same thing as the hand coded variant that I got from the Gill paper. Might be good to get some external validation, but it's likely not a priority.

@wavefunction91
Copy link
Owner Author

I've released that we'll need to expose some runtime configurability in this modular design. The grids that are exposed right now are relatively simple (the only real configurable parameter is a linear scaling factor), but the DE grids require a bit more configurability (deciding where to cut off the infinite sum, etc). The specification of alpha for TA could fall under this type of configurability.

This kind of configurability will also be useful for folks trying to optimize grids programmatically.

@hjjvandam
Copy link

hjjvandam commented Jul 11, 2023 via email

@wavefunction91
Copy link
Owner Author

Thanks @hjjvandam, that's definitely the uniform trapezoid variant.

@wavefunction91
Copy link
Owner Author

@susilehtola Anything in particular that would be beneficial to roll up into this PR? I think this is more or less g2g.

@susilehtola
Copy link
Collaborator

@susilehtola Anything in particular that would be beneficial to roll up into this PR? I think this is more or less g2g.

I think this is g2g, especially since I want the changes to the Chebyshev routines in master... The modularization can continue in other pulls if necessary

@wavefunction91 wavefunction91 merged commit 376561d into master Jul 12, 2023
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.

Modularize radial rules

6 participants