mabarnes / moment_kinetics

Other
2 stars 4 forks source link

Fokker Planck optional features #225

Closed mrhardman closed 1 month ago

mrhardman commented 3 months ago

Pull request permitting simulations with additional collision model (numerical method) options. Based on #224, consider merging #224 first.

By default we set vperp_bc = "zero" and use_conserving_corrections = true.

A check-in test is provided for vperp_bc = "zero-no-regularity". Additional experience is required to determine if this option is reliable for general simulations.

Decisions/Actions required:

mrhardman commented 3 months ago

dfdvperp_and_conserving_experiment_plots.zip In this attachment I include plots which show the effect of turning on and off two two switches:

The results are (note that Sdot is not positive definite)

Overall this justifies the use of ad-hoc conservation terms for long-term stability, and raises the question of whether we need to use dFdvperp = 0 as an imposed boundary condition when the collision operator is implemented with FEM and has zero flux at the origin. dFdvperp = 0 was introduced to stabilise a simulation with a numerical diffusion which had a flux at vperp = 0 (i.e., it was d^2 F/dvperp^2).

mrhardman commented 3 months ago

Further tests with the cross-collision operator looking at the effect of imposing dFdvperp =0. In these tests no sources or sinks are used. There are three test cases here:

These results suggest that

mrhardman commented 3 months ago

Two new test cases demonstrating the success of the vperp_bc="zero-no-regularity" option:

mrhardman commented 2 months ago

Tests showing that using vperp_bc="zero-no-regularity" yields stable simulations with numerical dissipation

Tests of the laplacian_derivative!() function in calculus_tests.jl are added.

mrhardman commented 2 months ago

@johnomotani I think that the dirichlet_bc feature in the Gauss Legendre derivatives is breaking my tests. Are you using these arrays to solve 2nd order ODEs? If not, I would remove this and impose boundary conditions externally to the derivatives.

Assumed problem commit: https://github.com/mabarnes/moment_kinetics/commit/3cff3eb120b0633e1b41a5801c410341732afdce

EDIT: by making the dirichlet_bc option leave the lower endpoint of vperp alone, I can make my tests pass. However, I still would like some clarification on how these matrices are being used, as the boundary conditions should only be applied if K and L are being inverted. I don't expect this is the default. In my opinion, new matrices for inverting would be a better option.

mrhardman commented 2 months ago

Tests using commit 8bff6a5658e8e3012fb8c0bef81c01c02760fb22:

johnomotani commented 2 months ago

@mrhardman I think I must've added that 'feature' to make matrices including the boundary condition that I could invert for preconditioning some implicit solves. I think I'll end up adding together several of these matrices, and modifying the result to impose Dirichlet bc anway, so I think the 'Dirichlet-bc-in-the-matrix' can be removed. If it's useful I think you're right and having separate matrices would be the right thing to do.

mrhardman commented 2 months ago

@mrhardman I think I must've added that 'feature' to make matrices including the boundary condition that I could invert for preconditioning some implicit solves. I think I'll end up adding together several of these matrices, and modifying the result to impose Dirichlet bc anway, so I think the 'Dirichlet-bc-in-the-matrix' can be removed. If it's useful I think you're right and having separate matrices would be the right thing to do.

How would you like to proceed? Should I revert the commits that made this feature, or do you want to deal with this in a new PR where you look at the implicit solves?

EDIT: Or I could just make a new commit changing the default, and let you add new matrices at a later date.

johnomotani commented 2 months ago

Or I could just make a new commit changing the default, and let you add new matrices at a later date.

Yeah, that sounds good!

mrhardman commented 2 months ago

@johnomotani Happy for the merge to go ahead now providing the last commit passes the tests.