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

Geometry upgrade #166

Merged
merged 75 commits into from
Mar 15, 2024
Merged

Geometry upgrade #166

merged 75 commits into from
Mar 15, 2024

Conversation

mrhardman
Copy link
Collaborator

Generalisation of the treatment of the magnetic geometry to permit the later addition of non-trivial options.

To do:

  • magnetic mirror terms
  • magnetic drift terms
  • vperp advection (d B / dt /= 0)
  • anything else related

…ic coefficents are used to permit extending to more complex magnetic geometry. Input options (including defaults) are modified but the input parameters in the examples and runs .toml files are left to be modified in a future commit.
@mrhardman mrhardman requested a review from johnomotani December 4, 2023 16:31
@mrhardman mrhardman self-assigned this Dec 4, 2023
…ExB drift terms. Note that this means that ExB drifts will only be present if bzeta > 0, so the pitch input parameter must be set to less that 1. This changes significantly the usage of the code so far, which assumed that bzeta ~= 1, whilst allowing an input parameter to set bzed.
…access to the D0 derivative vector for chebyshev grids.
… geometry_option='1D-mirror'. Use the parameter DeltaB to set the change in the magnetic field strength from the centre to the edge of the domain. The exact functional form is chosen to be a quartic with dB/dz = 0 at the sheath entrance.
@mrhardman
Copy link
Collaborator Author

@slnewton @johnomotani 80cca93 my intention is to investigate the toy geometry introduced in the option in the linked commit.

@mrhardman mrhardman linked an issue Jan 4, 2024 that may be closed by this pull request
…rily, to restrict vperp advection to cases with r.n > 1, after relaxing this constraint in time_advance.jl, the remaining bugs in vperp_advection.jl were fixed.
…rdinate has coord.n = 1, but the corresponding advect[is].speed array is used in the calculation of another quantity (e.g. r.n = 1 for a 1D mirror example with vperp advection). If this is not done, then NaNs can be introduced by the uninitialised array.
@johnomotani
Copy link
Collaborator

Oh, one other thing - it would be nice to have a docs page (.md file in docs/src/) at least briefly describing the available geometry options.

@mrhardman
Copy link
Collaborator Author

Oh, one other thing - it would be nice to have a docs page (.md file in docs/src/) at least briefly describing the available geometry options.

Should this file be a new file or should the content go in the zz_geo.md file? Are you happy with the description of the geometry given in the corresponding ExCALIBUR report?

@johnomotani
Copy link
Collaborator

Should this file be a new file or should the content go in the zz_geo.md file?

A new file. The files without the zz_ prefix are hand-written documentation (e.g. https://mabarnes.github.io/moment_kinetics/dev/post_processing_notes/#Post-processing). To add a new one, create the file, and then put the filename in the Pages list in index.md.

The files with zz_ prefix are the auto-generated module documentation (from the docstrings in the Julia code). The zz_ prefix is so that they get listed (in the left-hand sidebar of the docs site) together in a group that comes after all the hand-written pages. For whatever reason that sidebar sorts things by filename, but that turns out to work pretty well for what we want.

Are you happy with the description of the geometry given in the corresponding ExCALIBUR report?

That'd be great.

@mrhardman
Copy link
Collaborator Author

Are you happy with the description of the geometry given in the corresponding ExCALIBUR report?

That'd be great.

Is there a preference for putting the documentation in the geo.jl module next to the source? This seems like a more sustainable option, but perhaps a link in the source to the docs page is enough to ensure it is read and modified when the source is updated?

These input parameters are no longer used. The uses were only setting
them to the default values anyway, so can just be deleted.
@johnomotani
Copy link
Collaborator

Is there a preference for putting the documentation in the geo.jl module next to the source? This seems like a more sustainable option, but perhaps a link in the source to the docs page is enough to ensure it is read and modified when the source is updated?

That is an option, and I agree that it's nice to keep the source code and docs together. The downside is that it's a little bit more annoying to write documentation in docstrings in .jl files than in a .md file (e.g. when writing maths, the $ characters all need to be escaped, and I think some other similar things). So whichever you like. If you decide to put all the docs into the source code file, it would still be nice to have a geo.md page that can be linked in the index that just links to the module docs (the link would be created by [`moment_kinetics.geo`](@ref)).

@mrhardman
Copy link
Collaborator Author

OK. I am happy with linking to external libraries. For now the main problem is that I cannot work out how to make or view the documentation locally without working on git. Is it really necessary to have webviewer? That may be a problem, as I do all my development on remote servers.

@johnomotani
Copy link
Collaborator

For now the main problem is that I cannot work out how to make or view the documentation locally without working on git. Is it really necessary to have webviewer? That may be a problem, as I do all my development on remote servers.

It should be possible to build a pdf version by doing julia --project make-pdf.jl in the docs/ directory, instead of julia --project make.jl, but that option doesn't get tested much so it might be broken. If you build the html documentation, you can just download the build subdirectory that it will create, and open build/index.html with a web browser.

@johnomotani
Copy link
Collaborator

PS the README.md for the docs might need to be updated. I think you need some setup before the docs will build along the lines of

$ cd docs/
$ julia --project
julia>]
pkg> dev ../moment_kinetics
pkg> dev ../makie_post_processing/makie_post_processing
pkg> dev ../plots_post_processing/plots_post_processing

After that (I hope!)

julia> include("make.jl")

or

$ julia --project make.jl

should work. As usual, I prefer the REPL version because when you need to edit the .md files and rebuild the docs, the second (and later) docs builds are fast because the code is already compiled.

@johnomotani
Copy link
Collaborator

Ah, no. When you run make.jl you don't need to do any other setup first - that script takes care of installing the packages for moment_kinetics, etc. already. I forgot I'd done that! make-pdf.jl doesn't (yet) have that feature though.

File to contain documentation of geometry and options
@mrhardman
Copy link
Collaborator Author

I have just added a new file https://github.com/mabarnes/moment_kinetics/blob/geometry-upgrade/docs/src/geometry.md. Please comment!

Whilst writing this I considered that an input namelist for the geometry option (like numerical dissipation) would be appropriate. @johnomotani do you agree and should this happen in this PR?

@johnomotani
Copy link
Collaborator

Whilst writing this I considered that an input namelist for the geometry option (like numerical dissipation) would be appropriate. @johnomotani do you agree and should this happen in this PR?

I think that should happen in general. This seems like a good time to do it for the geometry. Including the change in this PR would be good as we are already breaking the input options (removing Bzed, etc.) so better to finish breaking them before people start writing new input files.

Once you create the new section, it would be good to add the 'old' top-level options to removed_options_list (see b52f943) so people don't get surprised if old input files don't behave as expected.

@johnomotani
Copy link
Collaborator

I have just added a new file https://github.com/mabarnes/moment_kinetics/blob/geometry upgrade/docs/src/geometry.md. Please comment!

Nice, thank you!

…nput files to be compatible with this change. This changes is not backwards compatible.
@mrhardman
Copy link
Collaborator Author

Whilst writing this I considered that an input namelist for the geometry option (like numerical dissipation) would be appropriate. @johnomotani do you agree and should this happen in this PR?

I think that should happen in general. This seems like a good time to do it for the geometry. Including the change in this PR would be good as we are already breaking the input options (removing Bzed, etc.) so better to finish breaking them before people start writing new input files.

Once you create the new section, it would be good to add the 'old' top-level options to removed_options_list (see b52f943) so people don't get surprised if old input files don't behave as expected.

This is now done, please review my latest changes!

mrhardman and others added 4 commits March 14, 2024 09:22
Bring input parameter documentation up to date with the source.
Comment out print statement.
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.

Looks good to me. We can merge when the CI tests pass.

@mrhardman mrhardman merged commit a5752c6 into master Mar 15, 2024
17 checks passed
@johnomotani johnomotani deleted the geometry-upgrade branch March 15, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants