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

Final transfer version #306

Merged
merged 58 commits into from
Jan 18, 2025
Merged

Final transfer version #306

merged 58 commits into from
Jan 18, 2025

Conversation

LucasMontoya4
Copy link
Collaborator

@LucasMontoya4 LucasMontoya4 commented Jan 14, 2025

Transfer multi source function and PI controller, and also functionality to turn off kinetic distribution function evolution and use a fluid closure for the ions instead. Note that this still only speeds up code by factor of around 5, as a lot of functions operating on f are embedded in essential code for the fluid model. Hence, best way to speed things up is to make ngrid and nelement 1 for vpar.

LucasMontoya4 and others added 30 commits September 18, 2024 10:11
…nskii ion physics. Gyrokinetic flag replacement of the gyrokinetic_ions case will happen in a later commit, so right now the gyrokinetic option for ion_physics does the same as drift_kinetic.
…gyrokinetic_ions = true being the test, we see whether ion_physics == gyrokinetic_ions
…nnormalised pdf (then subsequent heat flux updates will be with the braginskii closure)
….jl to use the reference collision frequency to calculate braginskii heat flux
…mean free path) and braginskii heat flux boundary condition at walls, following Stangeby (though I haven't checked the subtracted part for conductive heat flux yet).
…small correction to ion braginskii heat flux boundary condition
…ocstring for collisionality_plots function in makie_post_processing
…hat run (for when heat flux in the simulation is calculated using the shape function - having this option for braginskii_ions would just plot the same thing twice)
…nd power in upstream external source. Might add higher powers later.
… braginskii one (which just overplots the old one, apart from the boundaries)
…hich is much clearer than just writing braginskii everywhere. Note that I will soon change the naming system to collisional_krook
…ource with an input guess, which is the source_strength option in the input file. This means when you restart a run from another that had a constant external source, that choice of starting point for the PI_controller source should mean the PI_controller doesn't have to do too much work (as long as the target amplitude is not very far from the current midpoint amplitude).
…tances of the name braginskii for ion functions to coll_krook. The electron braginskii fluid naming is unchanged for now.
… collisions module can be defined later in moment_kinetics.jl. This fixes the issue highlighted in the previous commit.
…amplitude of an energy source so that the temperature midpoint is kept at a desired level. A few notes on this for now: 1. The temperature of the source has to be higher than the desired temperature, otherwise it can never be made hot enough, no matter how high the input amplitude. 2. The temperature of the source actually needs to be at least DOUBLE the desired value, which I think is coming from a factor of 2 error somewhere - this should be fixed when I comb through all the stuff I've written to iron out these missing factors of 2 one day. 3. This currently only really works for the ion fluid case - something has gone wrong with the part that actually adds to the pdf. I'll try to fix this soon.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to keep a revisioned copy of these files you could just make a new directory to store them in. It looks like only examples/* is checked for *.toml files in the workflow run. https://github.com/mabarnes/moment_kinetics/blob/master/.github/workflows/examples.yml.

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 guess that's true, but I'm dumping them all in the publication_inputs folder anyway, so we can keep the examples folder clean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like only examples/* is checked for *.toml files in the workflow run.

BTW this is not true. The walkdir("examples/") function recursively runs over all the entries of all the subdirectories of examples/ as well as the directory itself. So @LucasMontoya4's solution is the only possible one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK! I think this is what I meant, but I see I was being imprecise. However, I have noticed that things might not work as described in the past. See commit 998c725. There I noticed that the fokker_planck examples with source terms were clearly not being checked as the source terms functions that they would have used were broken for those inputs.

LucasMontoya4 and others added 3 commits January 16, 2025 16:07
…ch consist of two ion sources each. Neither uses a PI controller. The latter two tests (that I have now commented out) would have the rank 0 process during an MPI run hang at the end, and not print out the completion of the test set. I have not specifically narrowed it down to the PI controllers themselves, but it is definitely the different test input that is causing the problem.
Seems to be causing an error with Python in plots_post_processing at the
moment. Don't know why. Maybe after the cache empties/resets somehow we
can add the caching back again?
@johnomotani
Copy link
Collaborator

johnomotani commented Jan 17, 2025

I found the hang when running the PI controller tests in parallel. It was in loading the data after the run. get_run_info_no_setup() passes a flag ignore_MPI=true to mk_input() to tell it to skip MPI-ish things because it's being called in serial (even if there are multiple MPI ranks present, like in the parallel tests). That flag needed to be passed through to setup_external_sources!() to avoid allocating shared-memory arrays or doing MPI operation when ignore_MPI=true. That was a bug I made when I wrote the initial PI controller, not one from this PR - just shows that we do need tests for everything!

Edit: since this is fixed, I also re-enabled the PI controller tests in multi_source_tests.jl.

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. Nice work @LucasMontoya4! I'll just add a couple of little tidy-up changes, then merge this once the tests pass.

moment_kinetics/src/external_sources.jl Outdated Show resolved Hide resolved
moment_kinetics/test/multi_source_tests.jl Outdated Show resolved Hide resolved
@johnomotani johnomotani merged commit 5d4c9a3 into master Jan 18, 2025
21 checks passed
@johnomotani johnomotani deleted the final_transfer_version branch January 18, 2025 12:52
@LucasMontoya4
Copy link
Collaborator Author

Thanks for doing this @johnomotani! I hope the code review didn't take too long...

@johnomotani
Copy link
Collaborator

@LucasMontoya4 it was fine - I didn't check all your input files though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants