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

Patched microphysics to quokka, and added primordial chem test #263

Merged
merged 210 commits into from
Apr 26, 2023

Conversation

psharda
Copy link
Contributor

@psharda psharda commented Apr 18, 2023

Continuation of #241 . Closes #104

psharda and others added 30 commits December 22, 2022 08:39
@BenWibking
Copy link
Collaborator

@psharda
Copy link
Contributor Author

psharda commented Apr 25, 2023

/azp run

1 similar comment
@psharda
Copy link
Contributor Author

psharda commented Apr 25, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Apr 25, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Apr 25, 2023

GPU tests pass too! The only one that remains queued is debug arm64 gcc but otherwise this PR can be reviewed now.

@psharda psharda requested a review from BenWibking April 25, 2023 21:44
src/Chemistry.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@BenWibking BenWibking left a comment

Choose a reason for hiding this comment

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

I added a few code style comments, and un-resolved a few clang-tidy messages that I think should be fixed.

The main thing is that there needs to be a check that the answer is 'correct'. It probably makes sense to evolve the simulation until it reaches chemical equilibrium, then you can compare the values with the equilibrium abundances. In terms of implementing that in Quokka, you can follow the example here:

void RadhydroSimulation<ContactProblem>::computeReferenceSolution(amrex::MultiFab &ref, amrex::GpuArray<amrex::Real, AMREX_SPACEDIM> const &dx,
.

@psharda
Copy link
Contributor Author

psharda commented Apr 25, 2023

I added a few code style comments, and un-resolved a few clang-tidy messages that I think should be fixed.

The main thing is that there needs to be a check that the answer is 'correct'. It probably makes sense to evolve the simulation until it reaches chemical equilibrium, then you can compare the values with the equilibrium abundances. In terms of implementing that in Quokka, you can follow the example here:

void RadhydroSimulation<ContactProblem>::computeReferenceSolution(amrex::MultiFab &ref, amrex::GpuArray<amrex::Real, AMREX_SPACEDIM> const &dx,

.

I will look at the clang-tidy comments again. Some of them were outdated. For several remaining clang-tidy comments, the code was borrowed from existing test problems - why were they not fixed in the existing tests? Clang-tidy should have provided the same comments in those tests too.

For the second part, I am not sure what you mean by chemical equilibrium - this is fundamentally non-equilibrium chemistry. But if we want to compare with, say, results of the Microphysics unit test, it would require increasing the density by hand as we did in the unit test (otherwise the chemistry will not evolve). The implementation would then be identical to what we have in the unit test. At this point, I would prefer finishing off this PR without this check and opening a new PR for it, because we have reached a level where a basic primordial chem implementation exists and works well on CPUs and GPUs. It also gets hard to manage bulky PRs with lots of commits, and we already closed one PR to begin the current one.

@BenWibking
Copy link
Collaborator

BenWibking commented Apr 25, 2023

For the second part, I am not sure what you mean by chemical equilibrium - this is fundamentally non-equilibrium chemistry. But if we want to compare with, say, results of the Microphysics unit test, it would require increasing the density by hand as we did in the unit test (otherwise the chemistry will not evolve). The implementation would then be identical to what we have in the unit test. At this point, I would prefer finishing off this PR without this check and opening a new PR for it, because we have reached a level where a basic primordial chem implementation exists and works well on CPUs and GPUs. It also gets hard to manage bulky PRs with lots of commits, and we already closed one PR to begin the current one.

It could go in a separate PR. I'm not sure what you mean by 'fundamentally non-equilibrium' - is the equilibrium timescale longer than a Hubble time or something? Otherwise, we should be able to evolve until the values reach a steady state.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
src/PrimordialChem/test_primordial_chem.cpp Show resolved Hide resolved
@BenWibking
Copy link
Collaborator

I will look at the clang-tidy comments again. Some of them were outdated. For several remaining clang-tidy comments, the code was borrowed from existing test problems - why were they not fixed in the existing tests? Clang-tidy should have provided the same comments in those tests too.

I didn't install clang-tidy as a pull request action until recently, so it has only reviewed PRs from the past few months or so. Anything written before then wasn't reviewed by clang-tidy.

@psharda
Copy link
Contributor Author

psharda commented Apr 26, 2023

I will look at the clang-tidy comments again. Some of them were outdated. For several remaining clang-tidy comments, the code was borrowed from existing test problems - why were they not fixed in the existing tests? Clang-tidy should have provided the same comments in those tests too.

I didn't install clang-tidy as a pull request action until recently, so it has only reviewed PRs from the past few months or so. Anything written before then wasn't reviewed by clang-tidy.

Okay, all clang-tidy issues are now resolved. The last few open ones (since yesterday) were changes I had already implemented, so I resolved them too.

@psharda
Copy link
Contributor Author

psharda commented Apr 26, 2023

/azp run

@psharda
Copy link
Contributor Author

psharda commented Apr 26, 2023

For the second part, I am not sure what you mean by chemical equilibrium - this is fundamentally non-equilibrium chemistry. But if we want to compare with, say, results of the Microphysics unit test, it would require increasing the density by hand as we did in the unit test (otherwise the chemistry will not evolve). The implementation would then be identical to what we have in the unit test. At this point, I would prefer finishing off this PR without this check and opening a new PR for it, because we have reached a level where a basic primordial chem implementation exists and works well on CPUs and GPUs. It also gets hard to manage bulky PRs with lots of commits, and we already closed one PR to begin the current one.

It could go in a separate PR. I'm not sure what you mean by 'fundamentally non-equilibrium' - is the equilibrium timescale longer than a Hubble time or something? Otherwise, we should be able to evolve until the values reach a steady state.

I mean 1.) the rate of forward and reverse reactions can be quite different. The ratio of the forward to the backward reaction can be as high as 10^(30). Chemical equilibrium would demand the two rates be equal. 2.) timescale for chemical composition to change is >= the freefall timescale.

@psharda psharda requested a review from BenWibking April 26, 2023 12:26
@BenWibking BenWibking added this pull request to the merge queue Apr 26, 2023
Merged via the queue into quokka-astro:development with commit 80dfe77 Apr 26, 2023
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.

add primordial chemistry network from Microphysics
2 participants