Refactor collision input options to make separate namelist for Krook and Fokker Planck collision operators. #202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and Fokker Planck collision operators. Attempt to rationalise the input parameters of each to make the specification of default behaviour as simple as possible. Introduce use_fokker_planck and use_krook as Bool which determine if the operators are advanced, and frequency_option to determine if manual or reference parameter specification is used. Reference parameter option is made available for Fokker Planck self collisions as a result. Defaults must only be stored in a single Dict in the setup function for the input parameters, which should make understanding the options much easier than reading moment_kinetics_input.jl. A check is added to the reading of the input_section to make sure that any missing keys have their values filled from the default dictionary.
…llision frequency at setup so that the sign of the frequency is all that needs to be checked in the rest of the code.
…to specify the default input dictionary passed into the collisions input struct.
… a single charged species is supported here and the documentation may need improving or changing when multiple charged species with different Z are supported.
johnomotani
reviewed
Apr 24, 2024
moment_kinetics/src/fokker_planck.jl
Outdated
| # begin default inputs (as kwargs) | ||
| use_fokker_planck = false, | ||
| nuii = -1.0, | ||
| frequency_option = "manual") |
There was a problem hiding this comment.
I think the Krook collisions and FP collisions should have the same default setting for frequency_option, either both "manual" or both "reference_parameters". At the moment Krook has "reference_parameters" and FP has "manual".
Should we vote on which to choose? I'd vote for "reference_parameters" as the default, to make it easier to have self-consistent inputs. I guess @mrhardman would vote the other way!
There was a problem hiding this comment.
I would prefer manual input as default, but I can understand your reasoning. Let's change the default to "reference_parameters" on the basis that I will remember to ask for the "manual" option.
…atch that of the Krook collision.
johnomotani
approved these changes
Apr 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes to permit the collision operator inputs to be kept in separate structs.
Making these changes allowed me to review the formulation for the inputs. To simplify the logic for specifying default behaviour, I introduce new "use_krook" and "use_fokker_planck" parameters that must be true for the operators to be used. Checks on use_krook and use_fokker_planck are done when setting up the advance Bools. The "frequency_option" is used to choose whether or not the collision frequency is specified manually or from reference parameters. I think that this formulation is more natural, but this may be a matter of taste. My concern with the previous formulation was that the same string use used to determine the way the frequency is calculated as well as whether or not the operator is used at all.
A by-product of these changes is that a collision frequency from reference parameters for the Fokker-Planck operator is now available.
The feature should now mean that defaults are only specified in a single location, and that further parameters can be easily added in an encapsulated way.
Feedback appreciated.