Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fokker Planck optional features #225

Merged
merged 37 commits into from
Aug 7, 2024
Merged

Conversation

mrhardman
Copy link
Collaborator

@mrhardman mrhardman commented Jun 28, 2024

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

  • Collisions that avoid enforcing the dFdvperp = 0 regularity condition. Set vperp_bc = "zero-no-regularity" to access this feature.
  • Collisions that time-evolve without imposing the ad-hoc correction terms. Set use_conserving_corrections = false to access this feature.

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:

  • Whether or not to refactor vperp_bc = "zero" functionality to NOT impose dFdvperp = 0. @johnomotani @mabarnes @LucasMontoya4 opinions?
  • Test vperp numerical diffusion with/without dFdvperp = 0.
  • Test vperp advection without imposition of dFdvperp = 0.

…ollision operator only (or any diffusion operator that has no flux at vperp = 0 -- so not the numerical vperp diffusion at present -- where zero boundary conditions are imposed at vperp = L but no condition is imposed at vperp = 0. Initial short tests suggests that Maxwellian relaxation problems might run successfully without the regularity conditions -TBC.
…serving_corrections" in the Fokker Planck input namelist.
@mrhardman mrhardman added the enhancement New feature or request label Jun 28, 2024
@mrhardman mrhardman requested a review from johnomotani June 28, 2024 14:41
@mrhardman mrhardman self-assigned this Jun 28, 2024
@mrhardman
Copy link
Collaborator Author

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

  • fokker-planck-relaxation-beam-init1: impose dFdvperp = 0, use ad-hoc numerical conservation
  • fokker-planck-relaxation-dfdvperp-test1: do not impose dFdvperp = 0, use ad-hoc numerical conservation
  • fokker-planck-relaxation-dfdvperp-test1-no-conserve: do not impose dFdvperp = 0, do not use ad-hoc numerical conservation

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

  • fokker-planck-relaxation-beam-init1: Sdot tends to zero, moments are conserved to ~ 1.0e-6 over 20 collision times
  • fokker-planck-relaxation-dfdvperp-test1: Sdot tends to zero (but oscillates around zero, going negative), moments are conserved to ~ 1.0e-12 over 20 collision times
  • fokker-planck-relaxation-dfdvperp-test1-no-conserve: Sdot goes negative, but approaches zero from below. Moments are not conserved, which changes of order unity by 20 collision times.

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).

Michael Hardman and others added 4 commits July 1, 2024 15:48
…ng dF/dvperp = 0. Simulation shows reasonable behaviour with density preserved and thermal speed hitting a steady-state value near 0.15, which is held for multiple collision times. Ti/Talpha = 1/40 was chosen to reduce strain on resolution.
…for other parameters and no underscore being used in the examples files, so remove it in the source for consistency.
@mrhardman
Copy link
Collaborator Author

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:

  • sd_test_impose_dfdvperp_zero_large_dt.zip: Here we use numerical conserving terms, and we impose dFdvperp =0. We use a timestep that is too large, and the system goes numerically unstable around t = 14. Note the strange behaviour of the thermal speed at this time, and that the system seems to recover a physical-looking solution, even though we would rather find out more easily that this run is broken when we look at the final time step.
  • sd_tests_large_dt.zip: Here we repeat the test above, without imposing dFdvperp =0 artificially in the time stepper. We still use numerical conserving terms, and the system still goes numerically unstable, but this time the system does not "recover", and we can easily tell that the solution is broken.
  • sd_df_tests_small_dt.zip: Here we reduce the time step size, whilst we still use the "zero-no-regularity" boundary condition, meaning that dFdvperp =0 is not imposed. Now the simulation runs without numerical instability and the thermal speed converges to a fixed value (determined by the stationary background distributions that the evolved species collides with).

These results suggest that

  • We should determine whether we ever need to impose dFdvperp = 0 artificially for vperp advection and numerical dissipation, as this is currently the default boundary condition.
  • We should consider changing the default vperp boundary condition, perhaps by refactoring the "zero" option to be the current "zero-no-regularity" option.

@mrhardman
Copy link
Collaborator Author

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

  • sd_df_source_sink_self_collisions_false.zip: A test set up with sufficient scale separation and losses at small v to find the canonical isotropic slowing down distribution function, with comparison against the analytical result. Self-collisions are neglected. The simulation runs for around 20 normalised times (note that the time scale is normalised to nu_alphae by the choice of input nuii).
  • sd_df_source_sink_self_collisions_true.zip: The same test as above, with self collisions. Note that the slowing-down analytical solution does not match so well for small velocities, in accordance with expectations.

mrhardman added 2 commits July 5, 2024 18:51
…pport laplacian_derivative!() for chebyshev points with direct differentiation calculation.
…especially associated with the Chebyshev pseudospectral method, which does not show good convergence even at large ngrid. The finite-element GaussLegendre method shows much better behaviour.
@mrhardman
Copy link
Collaborator Author

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 and others added 9 commits July 8, 2024 12:31
…rms in vpa and vperp, for testing of the compatibility of the pseudospectral and the finite-element methods for advection and collisons, respectively. The option is chosen by selecting the [geometry] option = "0D-Spitzer-test", and picking fixed constant values of Ez, dBdr, Er, and dBdr. The vperp_advection!() routine must update the z_advect and r_advect structs if nr == 1 and nz == 1 and the "0D-Spitzer-test" option is being used, leading to some duplication of code. This could be avoided if the speed updates happened outside the function where the advection terms are computed.
…ker-Planck time evolution. In this mode of operation, boundary conditions on the fluxes are imposed through the assembly of the finite-element method, and no additional zero-ing of the distribution function is carried out. Initial tests suggest that this mode of operation is at least as good as other tested options.
@johnomotani johnomotani changed the base branch from master to fix_collision_operator_scripts July 30, 2024 17:44
@johnomotani johnomotani changed the base branch from fix_collision_operator_scripts to master July 30, 2024 17:44
Copy link
Collaborator

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

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

Whether or not to refactor vperp_bc = "zero" functionality to NOT impose dFdvperp = 0.

I think this sounds like a good idea - switch so that the default vperp_bc = "zero" does not impose dFdverp = 0 and the option that does impose it has a different name ("zero-impose-regularity" maybe? I'd leave it up to @mrhardman to pick a good name).

moment_kinetics/src/input_structs.jl Outdated Show resolved Hide resolved
moment_kinetics/src/moment_kinetics_input.jl Outdated Show resolved Hide resolved
moment_kinetics/src/moment_kinetics_input.jl Outdated Show resolved Hide resolved
moment_kinetics/src/moment_kinetics_input.jl Outdated Show resolved Hide resolved
@mrhardman
Copy link
Collaborator Author

mrhardman commented Jul 31, 2024

@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: 3cff3eb

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.

Michael Hardman added 3 commits July 31, 2024 11:05
…pose bc on lower endpoint of a cylindrical coordinate.
…ro" now implies that zeros are imposed at the ends of the domain, "zero-impose-regularity" imposes dFdvperp = 0 at vperp = 0.
@mrhardman mrhardman requested a review from johnomotani July 31, 2024 11:36
@mrhardman
Copy link
Collaborator Author

Tests using commit 8bff6a5:

@johnomotani
Copy link
Collaborator

@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
Copy link
Collaborator Author

mrhardman commented Jul 31, 2024

@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
Copy link
Collaborator

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!

… solve with K and L matrices is suppressed (but can be revived in a custom script with the dirichlet_bc=true optional argument in setup_gausslegendre_pseudospectral()).
@mrhardman
Copy link
Collaborator Author

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

@mrhardman mrhardman merged commit b776ef2 into master Aug 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants