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

Anisotropic cosmic rays #132

Merged
merged 33 commits into from
Apr 27, 2022
Merged

Anisotropic cosmic rays #132

merged 33 commits into from
Apr 27, 2022

Conversation

ibutsky
Copy link
Contributor

@ibutsky ibutsky commented Dec 10, 2019

This merge adds cosmic ray (CR) physics to the Dedner MHD solver (HydroMethod = 4) as well as anisotropic CR diffusion and streaming. 

The bulk of CR advection is solved in hydro_rk/(Riemann_LLF_MHD, Grid_MHDSourceTerms, and Grid_UpdateMHDPrim).  CR diffusion and streaming are calculated in Grid_ComputeAnisotropicDiffusion.C and Grid_ComputeCRStreaming.C

I'm still working adding tests for CR transport and improving the documentation, but I think this is ready to start the review process!

@jwise77
Copy link
Contributor

jwise77 commented Jan 3, 2020

We had a bug recently introduced that made the tests fail when you submitted your PR. It was fixed yesterday in #131. Could you merge the latest changes from enzo-project:master so the tests can run again?

Copy link
Contributor

@aemerick aemerick left a comment

Choose a reason for hiding this comment

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

Apologies this took so long to get around to reviewing, but it overall looks good to me. Thanks for putting this in!!

Most of my comments (I think) are documentation / comment related, with a few asking for clarification on a couple things just because I wasn't sure what was going on. And a couple optimization suggests. Nothing major I hope.

Thanks again!!

(Also looks like merge conflict resolutions with the current enzo-project:master is needed)

doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
doc/manual/source/physics/cosmic_rays.rst Outdated Show resolved Hide resolved
doc/manual/source/physics/cosmic_rays.rst Show resolved Hide resolved
src/enzo/WriteParameterFile.C Show resolved Hide resolved
src/enzo/Grid_CRTransportTestInitialize.C Outdated Show resolved Hide resolved
src/enzo/Grid_ComputeCRStreaming.C Outdated Show resolved Hide resolved
src/enzo/Grid_ComputeCRStreaming.C Show resolved Hide resolved
src/enzo/Make.mach.CCA-hardy Outdated Show resolved Hide resolved
src/enzo/hydro_rk/Grid_MHDSourceTerms.C Outdated Show resolved Hide resolved
@ibutsky
Copy link
Contributor Author

ibutsky commented Aug 6, 2020

Thanks @aemerick! I just implemented your suggestions.

Copy link
Contributor

@clairekope clairekope left a comment

Choose a reason for hiding this comment

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

This PR has been open for a good long while and I'd like to see it be merged.
Everything in the code looks good to me. I would like to see annotated parameter files for the CR Shock Tube and CR Transport Test problem types in the run directory.

Andrew's requests have been addressed even though he is no longer available to give the "ok" in GitHub. Test suite fails because the latest Matplotlib has removed matplotlib._png as of 3.3.0. The docs fail to build on CircleCI but I was able to build them locally.

Note: Two parameters that were not added by Iryna, but were in the pre-existing CR
implementation, are not documented: CRmaxSoundSpeed and CosmologySimulationUniformCR.

@clairekope clairekope dismissed aemerick’s stale review July 9, 2021 17:45

All changes have been met

Copy link
Contributor

@gregbryan gregbryan left a comment

Choose a reason for hiding this comment

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

This looks very good! I made a number of suggestions that should be addressed, but once those are done (and they shouldn't be hard), this should be merged.

doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
src/enzo/CRShockTubesInitialize.C Outdated Show resolved Hide resolved
src/enzo/CRTransportTestInitialize.C Outdated Show resolved Hide resolved
src/enzo/hydro_rk/Grid_MHDSourceTerms.C Outdated Show resolved Hide resolved
src/enzo/hydro_rk/Grid_MHDSourceTerms.C Show resolved Hide resolved
src/enzo/hydro_rk/Grid_ReturnOldHydroRKPointers.C Outdated Show resolved Hide resolved
doc/manual/source/parameters/index.rst Outdated Show resolved Hide resolved
clairekope and others added 4 commits September 14, 2021 13:58
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
Co-authored-by: Greg Bryan <gbryan@astro.columbia.edu>
@clairekope
Copy link
Contributor

clairekope commented Apr 19, 2022

Tested locally against main branch (2b20d1) sans tests using Hypre; not cleared to merge
test_results.txt

@clairekope
Copy link
Contributor

Reverted Grid_InitializeUniformGrid.C; it had changes unrelated to CR functionality that just... broke Enzo! Tested push suite locally (without Hypre tests); cleared to merge

@clairekope clairekope merged commit aceb1cf into enzo-project:main Apr 27, 2022
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.

5 participants