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

add cooling documentation #97

Merged
merged 11 commits into from
Aug 30, 2024
Merged

add cooling documentation #97

merged 11 commits into from
Aug 30, 2024

Conversation

BenWibking
Copy link
Contributor

@BenWibking BenWibking commented Feb 13, 2024

This adds a Markdown file with documentation on the convention for CIE cooling curves that is expected by AthenaPK, as well as documenting the approximations used in calculating the mean molecular weight.

A script to plot two example cooling curves in the form expected by AthenaPK is provided.

@BenWibking BenWibking marked this pull request as ready for review February 13, 2024 17:01
@forrestglines
Copy link
Contributor

This might belong in another PR, but we could make some functional changes to AthenaPK to reduce errors and bring cooling and treatment of temperatures more inline with the code:

  • Use only 2 columns in the cooling table -- first is log10 temperature, second is log10 $\Lambda_{hd}$. This would avoid human errors in choice of metallicity.
  • Use code units in the cooling table. This would force users to consider the generation of these tables. However, it would place extra work on the user and would require regeneration of cooling tables if simulation units changed.

Another larger change that I don't think belongs in this PR would be to address temperature units:

  • Introduce a Temperature unit. Right now all other units are user-defined but all temperatures are fixed to Kelvin. This would take the form of a user defined value of $k_B$
  • Or Use $k_B T$ instead of $T$ everywhere, since $k_B T$ already exists in our system of units as a unit of energy. This would affect parameters for cooling and potentially the temperature column in cooling tables.

@BenWibking
Copy link
Contributor Author

This might belong in another PR, but we could make some functional changes to AthenaPK to reduce errors and bring cooling and treatment of temperatures more inline with the code:

  • Use only 2 columns in the cooling table -- first is log10 temperature, second is log10 Λhd. This would avoid human errors in choice of metallicity.

I would be in favor of this change.

I do not agree with the other proposed changes. In particular, I think Kelvins are the only physical temperature unit that anyone would ever use. I think requiring input in code units would make human error more likely.

@forrestglines
Copy link
Contributor

I do not agree with the other proposed changes. In particular, I think Kelvins are the only physical temperature unit that anyone would ever use. I think requiring input in code units would make human error more likely.

That's very fair, my programmer brain for consistency is usually wrong. There goes my dream of a code using Rankine though

@forrestglines
Copy link
Contributor

If we include the updated Schure Cooling table (or tables for different metallicities) I think we should include this notebook in some form, perhaps reduced down to a small script without the plots: schure_cooling.ipynb.txt

@BenWibking
Copy link
Contributor Author

I think a notebook with plots included would be good to include in the docs. Or maybe a docs/notebooks subfolder?

@BenWibking BenWibking requested review from bwoshea and pgrete August 22, 2024 19:14
@BenWibking BenWibking added the documentation Improvements or additions to documentation label Aug 22, 2024
@pgrete
Copy link
Contributor

pgrete commented Aug 29, 2024

I just updated the PR so that

  • the SD table is removed
  • the Schure table is updated (and the old one is emptied with a note)
  • Moved the doc to a single place including figures
  • Added the notebook to calculate the new Schure table
  • Added the necessary tables for the calculations/plots

I think this can go in now.
What do you think @BenWibking

@BenWibking
Copy link
Contributor Author

@pgrete Did you check in the notebook? I don't see it.

@BenWibking
Copy link
Contributor Author

The only thing I still would like to change is to only have a single column in each input file, rather than multiple columns corresponding to different metallicities. This way, you have to specify in the file name exactly what metallicity you want.

@pgrete pgrete force-pushed the BenWibking/cooling-docs branch from 803978b to 342352b Compare August 30, 2024 13:49
@pgrete
Copy link
Contributor

pgrete commented Aug 30, 2024

Alright, I added the notebook (and updated to generate the separated cooling tables) and also changed the code so that only a single metallicity is supported per file.

@pgrete
Copy link
Contributor

pgrete commented Aug 30, 2024

The 👍 is approval (did you check the code)?

@BenWibking
Copy link
Contributor Author

Yes, I meant approval. I looked at the code, but I didn't test it myself.

@BenWibking BenWibking enabled auto-merge August 30, 2024 17:47
@BenWibking BenWibking merged commit 524ca26 into main Aug 30, 2024
4 checks passed
@BenWibking BenWibking deleted the BenWibking/cooling-docs branch August 30, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants