Self-shielded Grackle table#1527
Conversation
Summary of ChangesHello @chongchonghe, 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 significantly extends the capabilities of the Grackle cooling table generation and resampling tools. It introduces the ability to create self-shielded cooling tables and provides an option to exclude photoelectric heating, which is crucial for simulations requiring dynamic heating rates. Furthermore, the updated script now computes and visualizes cooling lengths, offering deeper insights into the thermal properties of the gas. Highlights
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 primarily introduces the calculation and plotting of cooling lengths into the resample_grackle_cooling_tables.py script. A new compute_cooling_time function was added, and the resample_cooling_tables function was updated to utilize this, store the calculated cooling lengths in the output HDF5 file, and generate a phase plot of cooling length. The cooling_rate function in grackle_tables.py was modified to include an include_pe parameter, allowing photoelectric heating to be conditionally applied. New command-line arguments (--exclude_pe, --shield, --local) were added to resample_grackle_cooling_tables.py to control photoelectric heating, use shielded Grackle data, and manage local storage of downloaded tables, respectively, with updated example usage in the script's docstring. Review comments indicate that the compute_cooling_time function's docstring and inline unit comment incorrectly refer to 'cooling length' and 'cm' instead of 'cooling time' and 's', and that an unused variable Z_abs should be removed from the plotting section.
BenWibking
left a comment
There was a problem hiding this comment.
Overall, looks good. I think it would be useful to add an HDF5 attribute to the output file that says whether PE is included or not. Otherwise, it relies on the user to change the filename to memorialize this, and that could be easily forgotten. On the Quokka side, it should check whether this attribute is set or not, and turn on/off PE accordingly.
Good point. I'll do it.
This won't work, because turning on time-variable PE heating requires self-consistent SF and quite a few runtime parameters. Quokka can't just turn it on itself. However, I can add a check and throw an error if there is a mismatch between the cooling table and the problem setup. |
for more information, see https://pre-commit.ci
|
/gemini review |
|
@codex review |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Code Review
This pull request introduces the calculation and storage of 'cooling length' in resampled Grackle cooling tables, including the generation of a phase plot for visualization. The resample_grackle_cooling_tables.py script was updated with new command-line options for handling shielded Grackle data, enabling local storage of downloaded tables, and providing more comprehensive usage examples. A significant change involves integrating the include_pe (photoelectric heating) flag into the HDF5 metadata of the resampled tables and modifying the C++ codebase to read this flag. The C++ changes ensure that if star formation history (SFH) based photoelectric heating is enabled in the simulation, the loaded Grackle cooling table must explicitly not include photoelectric heating, preventing double-counting. The DataTable.hpp utility was updated to correctly parse this new metadata attribute from HDF5 files. Review comments highlighted two issues: a potential TypeError in the Python script due to float division (n_rho/2) where integer division (n_rho//2) is required for array indexing, and a suggestion to clarify the docstring for compute_cooling_time to refer to Edot as 'net volumetric heating rate' instead of 'net cooling rate' for better accuracy.
…uokka-astro/quokka into chong/cooling/grackle-shielded
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@BenWibking @markkrumholz I addressed all the comments. Ready for review. |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@BenWibking @markkrumholz Can you review this PR? |
|
@BenWibking @markkrumholz Can you approve and merge this PR? |
We used it in https://ui.adsabs.harvard.edu/abs/2023MNRAS.521.5972W/abstract and it produced what I would say are "vaguely reasonable" results. So I am hesitant to change it for the full-disk simulations, unless we can be sure that it won't make the agreement with MW observations worse, given that there is a lot of missing physics in the current version. What are your criteria for saying that the shielded table is better? |
|
We need the revised tables for the dwarf galaxy simulations, because the current tables do not function properly at low metallicity. We know this because @chongchonghe's sub-solar tall box sims using the old grackle cooling fail to form any stars at all, because the heating from the extragalactic UV / X-ray keeps the gas thermally stable even when the photoelectric heating rate is zero. So I think we will need these new tables for the dwarf sims at a minimum. |



Description
Use this command to generate a Grackle cooling table with self-shielding (https://grackle.readthedocs.io/en/latest/Parameters.html#data-files), excluding photoelectric heating to allow time/space-variable photoelectric heating rate.
More changes:
resample_grackle_cooling_tables.pyscript will also make a phase plot of cooling length as a function of gas number densitynand effective temperatureT/mu.Related issues
Are there any GitHub issues that are fixed by this pull request? Add a link to them here.
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.