Binary-pairing Coulomb collisions: improvements and optimization

[JustinRayAngus]

Description updated by @RemiLehe, using text by @JustinRayAngus:

This PR now just makes some improvements on the relativistic implementation of Coulomb scattering method by writing the formula for the normalized mean free path (s12) in a way that is directly analogous to the well-known non-relativistic formulation. This save flops by removing redundant calculations and improves on the readability of the Coulomb collision algorithm.

The expression for the normalized scattering path in UpdateMomentumPerez.H is written as:
s12 = sigma_eff * n12 * dt * vrelst * g1sg2s/(g1g2)
with sigma_eff = pi*b0^2 * lnLmd

The proof that this expression is equivalent to Eq. 9 of Perez et al 2012 (https://doi.org/10.1063/1.4742167) is given here
CoulombScattering_s12.pdf

Previous description:

This PR originally addressed several issues with the binary-pairing Coulomb collision method in WarpX.

1. Optimization. See PR Avoid N² Operation in Binary-Pairing Coulomb Collsions #5066

2. Weighted-particle scattering physics. See PR Improving Coulomb collision method for weighted-particles  #5091. The method for doing weighted-particle scattering for Coulomb collisions prior to PR Improving Coulomb collision method for weighted-particles  #5091 is from Perez et al 2012 (https://doi.org/10.1063/1.4742167) which follows from Nanbu and Yonemura 1998 (http://dx.doi.org/10.1006/jcph.1998.6049). That method has been shown to result in incorrect relaxation rates for certain problems by Higginson et al 2020 (https://doi.org/10.1016/j.jcp.2020.109450). PR Improving Coulomb collision method for weighted-particles  #5091 fixes this issue using a method similar to that by Higginson 2020 that produces correct relaxation rates with weighted particles. Below are results from Tests T1a-T1d from Higginson 2020 (but with 10X less particles). The physical setup is the same for each of tests T1a-T1d, but they use different particle weighting setups for the different populations. In contrast to results obtained using the development branch, the results obtained using this PR show identical relaxation results for the various particle weight combinations.

Using this PR:

Using the development branch (prior to PR #5091):

[RemiLehe]

Thanks a lot for this PR! For clarity and because this PR will probably serve as reference in the future by other WarpX developers, would it make sense to split this into 2 independent PR?

• One PR that fixes the performance issue, by computing the density and temperature ahead of time.
• One PR that fixes the weighting issue, by introducing this code https://github.com/ECP-WarpX/WarpX/pull/5047/files#diff-9a3d3b7442d673878318b7ebdf440a46b42570bb3afae0766af6c2301e910dcfR118 and the automated test

[JustinRayAngus]

I understand and agree that your suggestion is ideal. It was my original intent to do them separately.

The issue is the n12 parameter computed in lines 99-108 of ElasticCollisionPerez.H. That value is an order N calc done for each binary pair (net cost is order N squared), but it's also not used at all in the improved method that produces the correct scattering. It seemed like a lot of unnecessary work to pull this parameter out to being computed at the cell level and then passing it down the chain of calls when it is not needed in the first place and all these added lines of code would just be removed in the follow on PR.

Unless you see a way to avoid the issue I described above, I think we should not split this PR into two. There will be a few more minor PRs related to Coulomb scattering following this one.

What do you think? Perhaps I can update the PR title to better reflect both parts of this PR?

[RemiLehe]

OK, thanks a lot for clarifying.

How about switching the order of the PRs, i.e. doing a separate PR that implements only:

• One PR that fixes the weighting issue, by introducing this code https://github.com/ECP-WarpX/WarpX/pull/5047/files#diff-9a3d3b7442d673878318b7ebdf440a46b42570bb3afae0766af6c2301e910dcfR118 and the automated test

(that way, the calculation of n12 is not needed in the code anymore)

and then once the above is merged, we could merge the current PR (i.e. #5047), which, at this point, will not show any changes for https://github.com/ECP-WarpX/WarpX/pull/5047/files#diff-9a3d3b7442d673878318b7ebdf440a46b42570bb3afae0766af6c2301e910dcfR118 since that part will already have been merged as part of the first PR.

Does that make sense, and does it sound realistic? If not, I agree we can also simply merge this PR (i.e. #5047) as a single combined PR.

[JustinRayAngus]

Splitting that way would be less work, but to be safe I should probably re-run all the tests (T1a-T1d), which will take a while because of the N^2 cost. If the aim of splitting the PRs in two is to be able to go back to the Nanbu method at some point for making comparisons or something, then I think it would make more sense to actually do it the first way you propose where the first PR is just about the optimization. That way also doesn't require re-running any of the new tests, since the code will fail for them anyhow.

For the sake of time and moving forward, I would advocate for simply merging this PR as is. This is my biased opinion. I'll let you have the final say of what to do.

[ax3l]

I like that split, that way we have a clear bug fix PR and a clear performance optimization PR. This will make also future potential regressions easier to spot and address (or reverts).

Our idea is not to go back to Nanbu in the future. It is more about having PRs that are easier to understand, to review and to go back to in the future.