beam-tracing / Scotty

Beam tracing code for diagnostics
https://scotty.readthedocs.io/en/latest/
GNU General Public License v3.0
7 stars 4 forks source link

Hamiltonian refactor #78

Closed ZedThree closed 1 year ago

ZedThree commented 1 year ago

(WIP till #77 is merged)

To do:

Refactor the Hamiltonian code (find_H and the find_dH_* functions). Deletes almost 4k lines and has a 30% speed-up.

90% of Scotty's run time is in evaluating the Hamiltonian, find_H. If we can make that faster, we can improve the performance of the whole code. This PR does that by roughly halving the number of times the Hamiltonian needs to be evaluated through reusing stencil points in the derivatives.

Instead of calling find_dH_* for each derivative we want, Hamiltonian.derivatives computes all the derivatives at once so that any common points can be reused between different derivatives.

For instance, $\partialR$ includes all the points for $\partial^2{RR}$, so we only need to evaluate $H$ at one more point and we can immediately calculate the second derivative. I played around with some fancier stencils for the mixed derivatives, but although they kept the error scaling at second order, they had a larger error coefficient, which resulted in solve_ivp taking more steps, wiping out any performance gains. I did use a slightly different stencil for the forward x central difference derivatives, like $\partial^2_{RK_R}$, as this did improve things overall.

How this works in practice:

The other change to the Hamiltonian is to pull out a PolarisationTensor class that caches $\epsilon\Vert, \epsilon\perp, \epsilon_g$ to be reused in calculating $\alpha, \beta, \gamma$

With these changes, we can then create a Hamiltonian object that contains some of the information that is reused, such as the field and density objects, and then we can pass much fewer arguments to the solver.

valerian-chen commented 1 year ago

Looks excellent, very excited for this. I've been thinking of reducing the redundant calls to evaluate H, but your solution is much more elegant than the few ideas I've had.

Just a minor note g and b are not parallel, so the components of eps should either be in the (x,y,g) basis or the (perp_1, perp_2,parallel) basis. If I understand you, it's probably the latter, but I'll go through the code carefully and figure it out. Also, at first glance, perhaps DielectricTensor rather than PolarisationTensor? I'll read through the code first and make sure I've got you, though.

ZedThree commented 1 year ago

I have almost certainly got at least some of the names wrong!

ZedThree commented 1 year ago

Comments and docs added. I've rebased on top of #77

I've renamed PolarisationTensor to DielectricTensor, but maybe the properties need renaming too?

ZedThree commented 1 year ago

I haven't encountered that term very much, but it is more accurate, yes. I'll make that change.

What about the components of DielectricTensor (the methods marked @property)? Are there better names for them?

valerian-chen commented 1 year ago

I've not forgotten this, I'm thinking carefully about the methods marked property. Hopefully I'll make up my mind soon

ZedThree commented 1 year ago

I've not forgotten this, I'm thinking carefully about the methods marked property. Hopefully I'll make up my mind soon

No rush! I'm refactoring the next thing :slightly_smiling_face:

valerian-chen commented 1 year ago

How does

eps_11, eps_12, and eps_bb or epsilon_11, epsilon_12, and epsilon_bb sound?

This is to keep the same names as equation 34 of https://iopscience.iop.org/article/10.1088/1361-6587/ac57a1/meta (Since that paper is also sort of documentation right now)

valerian-chen commented 1 year ago

Excellent, looking forward to the speed up!