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

Allow for non standard cosmologies, and improve cosmological time integration #400

Merged
merged 67 commits into from
Jul 24, 2024

Conversation

brantr
Copy link
Collaborator

@brantr brantr commented Jun 21, 2024

We want to include non-standard cosmological models that have time-dependent dark energy equation of state. This pull request implements w0,wa parameterization of the DE EOS. It also incorporates a Runge-Kutta integrator to improve the integration of the physical timestep with scale factor, pre-computes the offset in physical time for z<\infty ICs, and writes the expansion history to a file. We also allow for non-zero radiation energy density. I have checked the expansion history agrees to within ~0.1% with independent calculations.

brantr added 30 commits June 20, 2024 14:22
We need to track explicitly Omega_R (radiation), w0, and wa for time-variable dark energy models.  We also need to write to disk the expansion history, so we can verify that it's correct for unusual cosmologies. We've implemented much of that framework here, but will need to test.
Not sure why this is triggered for cosmology_functions.cpp but not io.cpp.
Added a computation of the offset in time at the start of the simulation, and a higher accuracy Runge-Kutta timestep scheme to improve the time integration given the typical scale factor step da.
Removing functionality just to get this to pass
brantr added 10 commits June 28, 2024 21:36
This checks the cosmological simulation grid units. Something similar is in Bruno's original cosmology version.
Removing the dens_number_conv a^3 scaling from chemistry_functions.cpp
Default dev branch does not match original cosmo code behavior.

Removing the eta_1 logic from hydro_cuda.cu and then changing the eta_1 default value to 0.001 seems to roughly follow Bruno's cosmo verison.
Changes were made to PPMP that affect reconstruction using the SIMPLE integrator, where "#ifdef CTU" was changed to "#ifndef VL". This affects the cosmological simulation hydro evolution, and behavior similar to Bruno's original code is recovered with this change reversed and SIMPLE integration used.
@brantr
Copy link
Collaborator Author

brantr commented Jul 1, 2024

After a lot of effort and help from Bruno, the version 1f05b8d reproduces well the original cosmological hydro sims using the same flags. The salient issue that may need discussion is the last commit that undoes changes to PPMP_cuda.cu, where ifdef CTU had been changed to ifndef VL. The last commit undoes this change, since it was affecting the cosmological hydro integration, and these portions of the PPMP scheme now only apply to CTU again (not VL or SIMPLE). @evaneschneider do you know why this change was made? Is it OK to keep this off whenever SIMPLE is used, or should I add an ifndef COSMOLOGY and revert the changes back to ifndef VL?

@evaneschneider
Copy link
Collaborator

@brantr I do think this needs some discussion. Perhaps part of the problem is that I don't really understand what SIMPLE should do. As I've mentioned a couple times, I think SIMPLE is numerically unstable in more than 1D, since I thought it was just the first half of CTU, and removing the characteristic projections and time evolution of the interface states makes it numerically unstable in 1D as well, so I probably thought I was helping when I noticed that step had been eliminated. (I do remember showing some KH tests in the CAAR slack that demonstrated instabilities that went away when the characteristic projection step was turned back on.) That said, if you all have worked out that simple works without those steps, we can (and perhaps should) get rid of all of that code (and I think we should be consistent here, either we have it in all the reconstructions, or we have it in none). I think resolving this question is best done through a conversation, since it seems like maybe you all did some development work in creating simple that I'm unaware of / do not understand.

@brantr
Copy link
Collaborator Author

brantr commented Jul 2, 2024

Let's discuss offline, but then when we resolve we can post back here for prosperity.

@evaneschneider
Copy link
Collaborator

When you get a chance @brantr can you re-run clang-format? I was going to try to investigate the linting error but right now there are a bunch of violations in a bunch of files.

@evaneschneider
Copy link
Collaborator

@brantr Are you sure you're running the right version of clang-format? There are still a bunch of errors in a number of files, but a lot of them seem to be simple things (like not removing end spaces or not getting equalities in line) that should just be taken care of when you run the formatting, so I don't really understand why it's not working.

@evaneschneider
Copy link
Collaborator

BTW, other than the formatting errors, everything seems okay on this branch except the new cosmology system test, which makes sense. Given that I assume this version has been verified for some forms of correctness, I'm happy to merge this and then rerun the system test and swap out the test data.

@evaneschneider
Copy link
Collaborator

pre-commit.ci autofix

@mabruzzo
Copy link
Collaborator

mabruzzo commented Jul 3, 2024

Oh wow! it looks like it precomit worked! That's extremely encouraging!

@evaneschneider
Copy link
Collaborator

Regarding formatting, thanks to @mabruzzo's PR #404 we now have the ability to fix formatting errors on PRs, so I went ahead and applied those changes.

@evaneschneider evaneschneider merged commit f525243 into dev Jul 24, 2024
12 of 14 checks passed
@brantr brantr deleted the non-standard-cosmologies branch July 24, 2024 21:49
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