Implement photoionization and add tests#1719
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements photoionization in Quokka, updates the Microphysics hash, and adds one-zone and Stromgren sphere test problems. The changes introduce a new photochemistry module, enhancing Quokka's ability to simulate astrophysical phenomena involving radiation and matter interactions. The included tests help ensure the correctness and stability of the new features. Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements photochemistry and adds corresponding tests. The changes include modifications to the main simulation loop and EOS to support a new PHOTOCHEMISTRY flag, and adds new networks for photoionization. The implementation of the photochemistry step as an operator-split source term seems correct. My main feedback is regarding code duplication in the new network definitions, which could be refactored for better maintainability. I've also pointed out several places with commented-out code that should be cleaned up before merging.
Note: Security Review did not run due to the size of the PR.
cb1ce31 to
a04ee2e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a major update to the project's development infrastructure, including a new quokka CLI tool, a complete migration of input files to TOML format, and extensive documentation updates. Feedback points out syntax errors in several new TOML files regarding array formatting and missing required parameters. Furthermore, portability issues were identified in the regression scripts due to hardcoded absolute paths and local directory references.
|
Ok, something went wrong with a merge into this branch. Essentially the last several weeks of PRs have somehow been included here as new commits. |
724812d to
64d1193
Compare
|
Is it fine now? I had performed a merge from dev to my branch for some tests to pass, since I had branched from a very old commit of dev, and after that format.h had been removed causing tests to fail. I have now rebased, so the commit history should look normal. |
Yeah, it looks fine now. |
|
This looks really good in general! I made some quick comments and will continue to review tomorrow. |
|
Thanks ChongChong! I'll make the changes suggested by you and Ben towards the end collectively once you have finished reviewing. |
|
@claude review |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
This is what Codex says: |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
…ry (quokka-astro#1892) ### Description Add support for reduced speed of light approximation, and test coupling of photochemistry with hydro by a D-Type test. ### Related issues Dependent on quokka-astro#1719 ### Checklist _Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an `x` inside the square brackets `[ ]` in the Markdown source below:_ - [x] I have added a description (see above). - [x] I have added a link to any related issues (if applicable; see above). - [x] I have read the [Contributing Guide](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md). - [x] I have added tests for any new physics that this PR adds to the code. - [x] *(For quokka-astro org members)* I have manually triggered the GPU tests with the magic comment `/azp run`. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: ChongChong He <chongchonghe99@gmail.com> Co-authored-by: Quokka Dev <rsla@quokka-dev.example.com>
### Description Implements photochemistry in quokka and changes the Microphysics hash to a compatible version. Also adds the one zone test and the stromgren sphere test problem ### Related issues Depends on AMReX-Astro/Microphysics#1975 ### Checklist _Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an `x` inside the square brackets `[ ]` in the Markdown source below:_ - [x] I have added a description (see above). - [x] I have added a link to any related issues (if applicable; see above). - [x] I have read the [Contributing Guide](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md). - [x] I have added tests for any new physics that this PR adds to the code. - [ ] *(For quokka-astro org members)* I have manually triggered the GPU tests with the magic comment `/azp run`. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: ChongChong He <chongchonghe99@gmail.com> Co-authored-by: Ben Wibking <ben@wibking.com>
…ry (#1892) ### Description Add support for reduced speed of light approximation, and test coupling of photochemistry with hydro by a D-Type test. ### Related issues Dependent on #1719 ### Checklist _Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an `x` inside the square brackets `[ ]` in the Markdown source below:_ - [x] I have added a description (see above). - [x] I have added a link to any related issues (if applicable; see above). - [x] I have read the [Contributing Guide](https://github.com/quokka-astro/quokka/blob/development/CONTRIBUTING.md). - [x] I have added tests for any new physics that this PR adds to the code. - [x] *(For quokka-astro org members)* I have manually triggered the GPU tests with the magic comment `/azp run`. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: ChongChong He <chongchonghe99@gmail.com> Co-authored-by: Quokka Dev <rsla@quokka-dev.example.com>
Description
Implements photochemistry in quokka and changes the Microphysics hash to a compatible version. Also adds the one zone test and the stromgren sphere test problem
Related issues
Depends on AMReX-Astro/Microphysics#1975
Checklist
Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an
xinside the square brackets[ ]in the Markdown source below:/azp run.