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

Krook 'collisions' #127

Merged
merged 53 commits into from
Nov 1, 2023
Merged

Krook 'collisions' #127

merged 53 commits into from
Nov 1, 2023

Conversation

johnomotani
Copy link
Collaborator

@johnomotani johnomotani commented Sep 11, 2023

Add a Krook collision operator. Needed this to make 1D1V simulations look nicer for EFTC!

Also includes a script to print some dimensional parameters from a simulation (e.g. ion-ion collision frequency).

TODO:

  • test - at least a regression test, including the moment-kinetic versions, is needed

@johnomotani johnomotani added the enhancement New feature or request label Sep 11, 2023
@mrhardman
Copy link
Collaborator

I have separately developed Krook collision operator code for 1V and 2V, and extended the manufactured solutions tests to accommodate this feature. The relevant commits are

6bd406e
d5c366f
97c788e
a6f06cc

Base automatically changed from makie-unnormalised-f to master September 13, 2023 11:26
These require interpolation in the velocity grids to compare to the
expected distribution function values.
Test is based on the NonlinearSoundWave test, but with Krook collisions
added. Only test Chebyshev derivatives (not finite difference) to save
time, as derivatives are not used in the Krook collision operator.
If there is a vperp dimension (i.e. ion distribution function is not
marginalised over vperp) then need a prefactor of 1/vth^3 rather than
1/vth (as for the 1V case) in front of the Maxwellian.
@johnomotani
Copy link
Collaborator Author

@mrhardman I'm proposing to merge this PR, as this Krook collision operator supports moment-kinetic modes, and has a density/temperature dependent collision frequency. Are you OK with that? It will need some merging with your MMS tests when you make a PR for those.

@mrhardman
Copy link
Collaborator

@mrhardman I'm proposing to merge this PR, as this Krook collision operator supports moment-kinetic modes, and has a density/temperature dependent collision frequency. Are you OK with that? It will need some merging with your MMS tests when you make a PR for those

I would prefer the MMS test options to be included now, since it seems that you only have a regression test here. The MMS tests are quite orthogonal to the rest of my collision operator development in that branch. Can we discuss offline what the barriers to including the MMS tests are?

@johnomotani
Copy link
Collaborator Author

@mrhardman I don't see any barrier to adding the MMS test. Please feel free to push it onto this branch. I wanted to add the regression test as something that would be run in the CI, as we don't have that set up for any of the MMS tests yet, and they may be too computationally heavy to run in CI anyway.

src/krook_collisions.jl Outdated Show resolved Hide resolved
@@ -197,27 +92,26 @@ function mk_input(scan_input=Dict(); save_inputs_to_txt=false, ignore_MPI=true)
composition.Er_constant = get(scan_input, "Er_constant", 0.0)

# Get reference parameters for normalizations
reference_parameter_section = set_defaults_and_check_section!(
reference_parameter_section = copy(set_defaults_and_check_section!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably the wrong place to discuss this, but I don't understand why we need reference parameters in the code itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reasoning is that we need the reference parameters to calculate the collision frequency, so for me it's easier to understand if it's explicit what the reference parameters are for a given simulation (i.e. given in the input file or set by defaults), and 'everything else' is forced to be consistent with them. At the moment it's 'just' (I guess?) rhostar and collision frequency at reference parameters that are actually being used, but at some point we might need/want for example atomic rates (ionization, charge exchange, etc.) that are tabulated functions of density and temperature.

Another motivation is that we might want a post processing routine that calculates things in SI units, like the heat flux to the target (in MW/m^2), or the power input into the simulation (in MW). Then we'd need to know what the reference parameters are, so it's simplest if they are explicitly specified.

What's the downside of having reference parameters specified?

else
vth_prefactor = 1.0 / vth^3
end
nu_ii = collisions.krook_collision_frequency_prefactor * n * T^(-1.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This choice of prefactor is probably motivated by the physical form of Fokker-Planck collision frequency, but it is not obvious to me that we would actually want this feature. Isn't it easier to understand the ad-hoc feature that we add if it doesn't have a nonlinear collision frequency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense to have both options (e.g. Gkeyll does that https://gkeyll.readthedocs.io/en/latest/gkyl/App/Collisions/collisionModels.html). If you want to add an option for constant collision frequency I'm happy for that to be there. I wanted to have a density/temperature dependent collision frequency because in a setup where physically the collisionality changes significantly (e.g. due to a large temperature drop between midplane and target) I don't want to have unphysically strong collisions upstream, but want the collisions near the target to be strong enough to Maxwellian-ise the plasma.

@mrhardman
Copy link
Collaborator

I am starting to merge in the appropriate features. The job may take some time as even the manufactured solutions module appears to have diverged from the initial state where d5c366f was committed to. If this merge is urgent, we should discuss.

src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
else
vth_prefactor = 1.0 / vth^3
end
nu_ii = collisions.krook_collision_frequency_prefactor * n * T^(-1.5)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense to have both options (e.g. Gkeyll does that https://gkeyll.readthedocs.io/en/latest/gkyl/App/Collisions/collisionModels.html). If you want to add an option for constant collision frequency I'm happy for that to be there. I wanted to have a density/temperature dependent collision frequency because in a setup where physically the collisionality changes significantly (e.g. due to a large temperature drop between midplane and target) I don't want to have unphysically strong collisions upstream, but want the collisions near the target to be strong enough to Maxwellian-ise the plasma.

@@ -197,27 +92,26 @@ function mk_input(scan_input=Dict(); save_inputs_to_txt=false, ignore_MPI=true)
composition.Er_constant = get(scan_input, "Er_constant", 0.0)

# Get reference parameters for normalizations
reference_parameter_section = set_defaults_and_check_section!(
reference_parameter_section = copy(set_defaults_and_check_section!(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reasoning is that we need the reference parameters to calculate the collision frequency, so for me it's easier to understand if it's explicit what the reference parameters are for a given simulation (i.e. given in the input file or set by defaults), and 'everything else' is forced to be consistent with them. At the moment it's 'just' (I guess?) rhostar and collision frequency at reference parameters that are actually being used, but at some point we might need/want for example atomic rates (ionization, charge exchange, etc.) that are tabulated functions of density and temperature.

Another motivation is that we might want a post processing routine that calculates things in SI units, like the heat flux to the target (in MW/m^2), or the power input into the simulation (in MW). Then we'd need to know what the reference parameters are, so it's simplest if they are explicitly specified.

What's the downside of having reference parameters specified?

2V Krook collisions are now implemented for full-f simulations.
@mrhardman
Copy link
Collaborator

What's the downside of having reference parameters specified?

So long as it is clear that all evolved quantities are normalised, I will have to be happy. In HPC codes that I have worked with previously all small numbers are eliminated from the equations in favour of normalised parameters which enter the input file, and are computed externally to the HPC code. This also gave the option of making physics studies where unphysical parameter scans can be supported (changing the mass of particles, but not collision frequency, for example). In an ideal implementation we would use all reference parameters in a separate module and not propagate their use into the main code, where only normalised quantities are used. This might be what you are already doing -- in which case I cannot find fault.

src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
This option now passes the tests, so might as well make it the suggested option.
Krook collisions manufactured solutions test port
Copy link
Collaborator Author

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

Make clearer that T_over_Tref is not the normalised temperature as defined in the code.

src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
src/krook_collisions.jl Outdated Show resolved Hide resolved
...with the current normalisations used by the code, which normalise pressure by `mref*Nref*cref^2 = 2*Nref*Tref`.
@johnomotani johnomotani requested a review from mrhardman October 30, 2023 20:07
@mrhardman
Copy link
Collaborator

Make clearer that T_over_Tref is not the normalised temperature as defined in the code.

Why not avoid using T_over_Tref entirely and put

n * vth^(-3)

in the collision operator factor?

Copy link
Collaborator

@mrhardman mrhardman left a comment

Choose a reason for hiding this comment

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

Regarding making the "reference parameters" option default for the MMS tests: Do we have a diagnostic output for what the actual collision frequency used is for this option? This could be helpful for understanding whether or not the inputs give O(1) coefficients in the MMS test. I think the default ended up being ~ 0.95.

@johnomotani
Copy link
Collaborator Author

Implementing collision operator in terms of vth - I like that idea. I guess however we end up defining cref, we'd choose to have vth(norm) = vth(unnorm)/cref, so the implementation in terms of vth would keep working for any choice.

Diagnostic output for collision frequency - that would be useful, and should be an easy thing to implement.

This means that to calculate the normalised, spatially-varying collision
frequency we multiply by (the normalised) n/vth^3, and do not need to
worry about potentially changing normalisation of T (to mref*cref^2 or
to Tref).
Makes it easier to reuse in future (e.g. for post processing).
Need to add an `element_spacing_option` argument for the dummy
coordinate input.
...rather than handling ad-hoc in `plots_for_variable()`. This is
cleaner, and makes it possible when using makie_post_processing
interactively to easily get "temperature", "collision_frequency", etc.
@johnomotani
Copy link
Collaborator Author

...both upgrades now made.

After adding a function that calculates collision frequency (and a little bit of refactoring of makie_post_processing), adding plots of the collision frequency in makie_post_processing only took 9 lines 😎

"collision_frequency")

elseif variable_name == "collision_frequency"
n = postproc_load_variable(run_info, "density")
vth = postproc_load_variable(run_info, "thermal_speed")
variable = get_collision_frequency(run_info.collisions, n, vth)

elseif variable_name == "collision_frequency" &&
all(ri.collisions.krook_collisions_option == "none" for ri run_info)
# No Krook collisions active, so do not make plots.
return nothing

@johnomotani
Copy link
Collaborator Author

PS the spatially-varying collision frequency is O(1), at least in the 1V test
1D-wall_MMS_new_nel_r_1_z_16_vpa_16_vperp_1_krook_collision_frequency_spec1_vs_z

@johnomotani johnomotani merged commit fc6da74 into master Nov 1, 2023
8 of 21 checks passed
@johnomotani johnomotani deleted the Krook-collisions branch November 1, 2023 12:37
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