Inputs refactor and extra checking #247
Conversation
Also tidy up the coordinate initialisation.
recursive_merge() is more flexible, as it can handle arbitrarily deep nesting of Dicts. Also move merge_dict_with_kwargs() to utils module, as it is only needed in the tests, not within the moment_kinetics module itself.
The Github CI check for the examples does not use distributed MPI, so nelement_local should not be set in the examples.
This tests a feature that was never added to moment_kinetics.
Create a section to contain the `force_Er_zero_at_wall` option (and others in future). Remove the unused `drive_input` structs.
This is probably more confusing than helpful. By removing it, all dissipation coefficients have to be set explicitly.
Unexpected section names are probably typos, so we error so that the user can fix them rather than having any options in them be silently ignored.
This setting is now in the [timestepping] section.
This variable is no longer only used to set up parameter scans, so give it a more logical name.
...so that it at least works for the 1D1V case, and errors otherwise.
A current test case (long tests in restart_interpolation_tests.jl, before adding a workaround for the run_name length in that test) crashes when the filename is 245 characters, and not when the filename is 243 characters.
|
PS this PR also adds a CI job that checks the scripts in |
|
This PR is pretty big because of all the changes to input and test files, but for the source code files in |
| neutral_input_dict = Dict(Symbol(k)=>v for (k,v) in neutral_input) | ||
| neutral_params = neutral_num_diss_params(; neutral_input_dict...) | ||
|
|
||
| return numerical_dissipation_parameters(ion_params, electron_params, neutral_params) |
There was a problem hiding this comment.
Why replace this struct with a NamedTuple? Since this data is clearly getting passed around, it would seem useful to make this data something that cannot be modified afterwards.
There was a problem hiding this comment.
NamedTuple is immutable.
I want to use set_defaults_and_check_section!() to read the inputs, because that's the function that registers the section as being 'expected' so we can check that only expected sections exist. Also, set_defaults_and_check_section!() has nicer error messages.
There was a problem hiding this comment.
I have nothing against set_defaults_and_check_section!(). My concern here was just about the object returned by the function. For geometry and composition and etc, we use explicitly defined structs. I find these easier to understand because the typing documentation is explicitly given somewhere. Happy to hear that NamedTuple is immutable, would be even happier if there was an explicit type definition of everything that goes into the NamedTuple (i.e., it was a struct : ) ). This is probably just a preference based on the other existing namelist options.
There was a problem hiding this comment.
Yeah, there's not quite an ideal solution. I'd like to only 'declare' the options in one place - either as the fields of a struct or as the arguments to set_defaults_and_check_section!(). The trouble is that: if you declare the fields and the defaults in a struct definition, then the defaults have to be constants; if you use get_defaults_and_check_section!() to get an OptionsDict with the right variables, you'd have to declare them twice to have a struct that contains the same variables.
Using NamedTuple is a compromise where you do the second option, but have to live with having an implicitly-defined NamedTuple rather than an explicitly-defined struct. Sometimes having the names declared twice so you can have the struct will be the best thing to do.
See the new definition of initial conditions in 3ad41d6 for an example of the 'first option'.
There was a problem hiding this comment.
I acknowledge the two options, and suggest a third -- my preference is for an option always involving an explicitly defined struct, at the expense of declaring the variables twice to get the right defaults, where necessary.
There was a problem hiding this comment.
That is an option, and we do use it in places. I find the NamedTuple clear enough (as the usage is pretty standard, the fields are defined in a call to set_defaults_and_check_section!() in a 'setup' function for some module) that it's worth using (sometimes) for the benefit of being easier to update.
I don't think we're going to agree on this. @mabarnes @LucasMontoya4 do you have an opinion? I think to sum up, the question is:
In the case where we want the default for one or more options to not just be a constant value (i.e. depend on another option, or some other parameters) there are two options
- Define a struct for settings (using
@kwdef); callset_defaults_and_check_section!()with the keyword arguments being the fields of the struct, creating anOptionsDict; do some processing to update the default values as necessary, modifying theOptionsDict; construct an instance of the struct using theOptionsDict. - Call
set_defaults_and_check_section!()with the keyword arguments being the fields of the struct, creating anOptionsDict; do some processing to update the default values as necessary, modifying theOptionsDict; useDict_to_NamedTupleto create aNamedTuple(which is an immutable, implicitly defined struct) from theOptionsDict.
1 has an explicitly declared struct, but requires all the fields to be 'defined' twice (once in the struct definition and once in the call toset_defaults_and_check_section!(). 2 defines the fields only once, but creates aNamedTuplewith those fields.
Should we require 1 or 2, or leave it up to whoever is writing the module to decide which seems more convenient?
PS in this particular instance, we could avoid this issue by having constant defaults if we didn't bother to replace -Inf with nothing for force_minimum_pdf_value. I think using nothing is marginally more efficient because the if can be evaluated at compile-time, but the cost will be totally negligible anyway because it's one float comparison that's outside any loop over arrays. Maybe that would be the best option in this instance!
There was a problem hiding this comment.
Actually, maybe this is not such a big issue after all. I got into the habit of using NamedTuples when set_default_and_check_section!() couldn't handle structs at all, but this discussion with @mrhardman prompted me to add the version that can directly create an @kwdef struct as long as the defaults are constant. Now (with the commits I just pushed) using that version where possible, there are only a few places where we create settings in a NamedTuple and:
evolve_momentssettings don't propagate far, only until they get put into themomentsstruct.- output from
get_coordinate_input()only gets used inside or fed intodefine_coordinate(). - input for manufactured solutions is only used within that module, and it's useful to make adding new inputs as simple as possible there.
- the
nonlinear_solversfunctionality is still WIP, so it's useful to be able to change it simply (at least for now). - The reference parameters aren't used in many places, and we might want to add more derived parameters.
Thanks to Patrick Farrell for guidance on periodic boundary conditions in finite element codes!
Helps to prevent special properties of test function (e.g. symmetry, or being zero on the periodic boundary - not sure what is actually relevant!) from masking errors in the implementation.
...without even rounding errors, as these might drift and accumulate into a non-negligible error after many timesteps.
Ensures an error will be thrown if `L_matrix` is used when it has not been implemented, and removes the need for a noisy warning.
… between periodic BC modifications required for matrices on LHS or RHS in LHS.x = RHS. The ODE solver in the script sets RHS[end] = 0.0 so second version of mass matrix is avoided. However, L_matrix_with_bc appears to be singular in some examples, suggesting a continuing problem.
…eriodic case, this is one to make the system periodic, and one to fix the constant offset. This should remove the problems with a singular L matrix.
As the input can be assumed to be periodic when using periodic bc, it is not necessary to zero out the last row of the 'rhs' matrix - it is ok to use a similar form (1 0 0 ... 0 0 -1) as the last row of the 'lhs' matrix, because the two non-zero entries will cancel exactly due to the periodicity. This is convenient because it means we do not need to distinguish between 'lhs' and 'rhs' matrices - we can use the same matrix for both functions.
Correct 1 d matrix assembly
johnomotani
force-pushed
the
coordinates-input-refactor
branch
from
3663d04 to
0405d08
Compare
The explicitly defined structs for numerical dissipation parameters were removed to use `get_defaults_and_check_section!()`, but now that `get_defaults_and_check_section!()` can handle an `@kwdef struct`, it makes sense to put the structs back.
johnomotani
force-pushed
the
coordinates-input-refactor
branch
from
0405d08 to
ba41f32
Compare
Having constant defaults rather than ones that depend on other input parameters makes the setup easier to define, and the previous defaults were not physically meaningful anyway.
The name for the 'stop file' used to be changeable in the timestepping options, but if it was set would have to be set with either an absolute path or a path relative to where `julia` is running, not relative to the output directory, so this option wasn't very helpful anyway. Now remove the option, and just hard code the name to "stop".
Also define a `setup_io_input()` function to simplify `mk_input()`.
Comment said "end user inputs. do not modify following code!" but some setup functions have to come later, e.g. because they use coordinates, and users have not had to modify the `mk_input()` function to change parameters for a long time.
johnomotani
force-pushed
the
coordinates-input-refactor
branch
from
ba41f32 to
0458ab0
Compare
NetCDF seems to break the parallel tests. Don't understand why.
When an `@kwdef struct` is passed to `set_defaults_and_check_section!()` so that the defaults are applied by the struct, also load the final values back into the `options` Dict so that they can be saved to the output file. Note that there is no null value in TOML, or in HDF5, so default values of `nothing` are not allowed, as these could not be saved. Throw a helpful error if `nothing` is found in options, also add a note in the docs about this. Some defaults in [numerical_dissipation], [timestepping], [electron_timestepping] and [output] sections had to be changed from `nothing`.
To ensure that we are notified of any warnings introduced, e.g. by mistakes in docstrings, so we know to fix them before we merge the PR and deploy the new docs.
Organize the rest of the options that were still at the top level of the input file into sections. It would be easy to handle top-level options if we wanted them (I actually already wrote
set_defaults_and_check_top_level!()for themakie_post_processingoption reader), but I didn't see at the moment any options that it didn't make sense to put in some section.All options are now declared using
set_defaults_and_check_section!()so we check that each option that is given in the input exists. This PR also addscheck_sections!()which checks that all the sections that are given in the input exist. This makes it impossible for a typo or out of date input file to mean an option that was meant to be set is not set - the 'worst' that could happen would be for a mistake to change the name of one option into a different option, but that should be pretty unlikely. At least nothing will be silently ignored now.While modifying the 'options', takes the opportunity to remove the remaining
mutable structdefinitions ininput_structs.jlto remove duplication (a couple of themutable structs were already not being used). The final options objects are still immutable, as they are (mostly)NamedTuples created withDict_to_NamedTuple().The new sections are:
[r],[z],[vperp],[vpa],[vzeta],[vr],[vz]for the coordinates inputs[em_fields]- so far just for theforce_Er_zero_at_walloption[evolve_moments]for the moment-kinetic flags, which are nowdensity,parallel_flow,parallel_pressure, andmoments_conservation[reactions]for the ion-neutral interaction parameters[electron_fluid_collisions]for thenu_eiparameter used by thebraginskii_fluidelectron modelrun_nameandbase_directorymove into the[output]sectionupdate-input-file.jl.zip is a (zip'ed) script that will convert old input files to the new format. You might want to run it to get a new input file, then copy over the new sections by hand to your original file, because the script will remove all comments and might reorder options and sections. It's currently written to rename the original file with a
.unmodifiedsuffix, but you could easily edit to add a suffix to the new filename instead if you prefer.There are a couple of minor changes to defaults: the defaults for
vzare no longer taken from the settings forvpa; the default forneutral_numerical_dissipation:vz_dissipation_coefficientis no longer taken fromion_numerical_dissipation:vpa_dissipation_coefficient. I don't think we often (ever?) relied on these defaults anyway, so hopefully this is not a disruptive change.Closes #241.
Edit 11/9/2024: replaced the input file updater script with a fixed version. Thanks @LucasMontoya4 for noticing the bug!