Skip to content

Conversation

@susilehtola
Copy link
Collaborator

@susilehtola susilehtola commented Jul 8, 2023

Per @loriab comment in #27, gausscheby1 had a wrong equation in the code. This is fixed by this PR.

This PR also converts all gausscheby routines to use the same indexing. In my opinion, it is best to choose the indexing such that the equations match the mathematical expressions, since this makes checking their correctness easier.

The third thing this PR does is to make sure that all Gauss-Chebyshev quadratures generate nodes in ascending order.

(The correctness bugs were already fixed in #32.)

@loriab
Copy link

loriab commented Jul 8, 2023

for abcissas for the third rule at #27 (comment) you have cos^2 whereas here in the code you have plain cos. Otherwise all three lgtm. I like the clearer indexing.

@wavefunction91
Copy link
Owner

+1 for the clearer indexing, but the bugs were fixed in #32. I'll address @susilehtola 's comments there and get that merged, and we can address the indexing with this PR

Copy link
Owner

@wavefunction91 wavefunction91 left a comment

Choose a reason for hiding this comment

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

As discussed, this should take #32 as its base. Also, while were on the topic of indexing, we should address the the problem of ascending vs descending ordering. It needs to be consistent. I know Psi4 does descending, but I'm not a huge fan of that for a number of reasons. I prefer to keep everything ascending to make life easier in the generation of pruned angular quadratures. For some quadratures, this can be done by simply swapping a sign of the point, for others its not so obvious. Probably best to just handle it by indexing redirection.

@loriab
Copy link

loriab commented Jul 8, 2023

I don't know if you're needing a comment for the Psi4 perspective, but I'm not following what ascending and descending refer to.

@wavefunction91
Copy link
Owner

@loriab As in having the quadratures stored in increasing (ascending) or decreasing (descending) order - Psi4 assumes the latter, whereas I prefer to work with the former

The defaulting ordering is descending:

for(int i = 1; i < n; ++i) {
  auto xi = cos( M_PI * i / (n+1) ); // x[i] > x[i+1] for i in [0,n)
}

For GC-{1,2}, you can fix the order by just swapping the sign since the quadratures are symmetric. For GC-2-mod and GC-3, it's not so clear due to the change in variables.

@susilehtola
Copy link
Collaborator Author

susilehtola commented Jul 9, 2023

Rebased on top of #32. Indexing is now consistent, and all rules generate nodes in ascending order. I also added a test for the correct ordering; it turns out that all the rules gave points in the opposite order.

@wavefunction91
Copy link
Owner

@susilehtola Update with master (there's a minor conflict) and this is g2g.

and weights match the mathematical expressions. Moreover, ensure that
the nodes are returned in increasing order. Also fix some typos in the
comments.
@wavefunction91 wavefunction91 merged commit 62a23d1 into wavefunction91:master Jul 9, 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.

3 participants