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

Fix missing mu in temperature calculation RAMSES #4817

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

V-Nathir
Copy link

@V-Nathir V-Nathir commented Feb 14, 2024

PR Summary

The YT´s output for Temperature was not exactly the temperature, it was temperature over mu. This mu is required since P/rho = kb/mu * T/m_p; where p=pressure, rho=density, T=Temperature, m_p=proton mass, kb=Boltzmann Cte., and mu is the dimensionless average particle weight. Yt used to do P/rho * mp/kb = T, this T is now T_over_mu, and the temperature is now T_over_mu * mu. Also, the dimension of mu has been changed.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

Copy link

welcome bot commented Feb 14, 2024

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

@cphyc cphyc added bug code frontends Things related to specific frontends labels Feb 14, 2024
@cphyc
Copy link
Member

cphyc commented Feb 14, 2024

Thanks for the PR @V-Nathir! I made some modifications to ensure that we are backwards compatible with old code versions. If we cannot read the cooling tables, we assume that T = T/µ (it is wrong, but we can't easily get µ) and we raise a warning when the user tries to access the temperature. For recent code versions, we interpolate µ from the cooling tables, T/µ and nH.

For reviewers: this is a breaking change but also a bug. I don't really see a way forward to fixing it that would satisfy these two constrains:

  1. the same code provides the same output as pre-bug versions,
  2. new users get the right temperature, i.e. we do not store the "fixed" version in a different field name, like "temperature_that_is_T_rather_than_T/mu"

@cphyc
Copy link
Member

cphyc commented Feb 14, 2024

Failure on the build on py3.12 is unrelated :)

@neutrinoceros
Copy link
Member

How about we just ship it in our next feature release instead of a bugfix release ?

@cphyc
Copy link
Member

cphyc commented Feb 14, 2024

Either way is fine by me.

cphyc
cphyc previously approved these changes Feb 14, 2024
Copy link
Member

@cphyc cphyc left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd like a second pair of eyes with someone from astro (@brittonsmith?)

@neutrinoceros
Copy link
Member

Either way is fine by me.

Then I should leave this choice to reviewers. Please remember to set a milestone before you merge this. Thanks !

@cphyc
Copy link
Member

cphyc commented Feb 15, 2024

The failing tests are unrelated and fixed in #4819.

@jzuhone jzuhone changed the base branch from main to yt-2.x February 20, 2024 19:28
@V-Nathir V-Nathir changed the base branch from yt-2.x to main April 12, 2024 10:07
@V-Nathir V-Nathir dismissed cphyc’s stale review April 12, 2024 10:07

The base branch was changed.

@cphyc cphyc added this to the 4.4.0 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug code frontends Things related to specific frontends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants