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 Stephan-Boltzmann constant accuracy and add radiation constant #520

Merged
merged 7 commits into from
Oct 4, 2024

Conversation

zingale
Copy link
Member

@zingale zingale commented Sep 28, 2024

and make it and Stefan-Boltzmann derived from fundamental units

and make it and Stefan-Boltzmann derived from fundamental units
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

I'm not against it, though I may have some reserves about using a single greek letter symbol for such a domain specific constant, but I do not understand why it's having an effect on existing doctests. Can you clarify ?

@@ -473,9 +473,9 @@ class EffectiveTemperatureEquivalence(Equivalence):
effective_temperature: flux <-> temperature
>>> from unyt import K, W, m
>>> (5000.*K).to_equivalent('W/m**2', 'effective_temperature')
unyt_quantity(35439831.25, 'W/m**2')
unyt_quantity(35439828.89119583, 'W/m**2')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure such a large change is warranted (unless it somehow counts as a bug fix), but why is this output changing at all ?

@zingale
Copy link
Member Author

zingale commented Oct 2, 2024

I didn't change the name of the constant -- it was already σ in the code.

What I did is fix the way the constant is computed. The Stefan-Boltzmann constant is not a fundamental constant -- it is constructed from Planck's constant, Boltzmann's constant and the speed of light (https://en.wikipedia.org/wiki/Stefan%E2%80%93Boltzmann_law#Stefan%E2%80%93Boltzmann_constant). So now its value is consistent with the values of the fundamental constants that are already in unyt.

The reason for the doctest change is that σ's value changed slightly, because it is now defined in terms of the fundamental constants.

@neutrinoceros
Copy link
Member

I didn't change the name of the constant -- it was already σ in the code.

Whoops, sorry I misread the diff

Ok thanks for clarifying, I missed that there were actually two changes here: fixing stefan_boltzmann_constant and adding radiation_constant.
Searching for the latter, I find https://en.wikipedia.org/wiki/Radiation_constant, indicating that the name is somewhat ambiguous. Could we use stephan_boltzmann_constant or radiation_density_constant instead ?

@neutrinoceros neutrinoceros added bug Something isn't working enhancement New feature or request labels Oct 2, 2024
@zingale
Copy link
Member Author

zingale commented Oct 2, 2024

radiation density constant is fine, although I've never seen it referred to anything other than the "radiation constant" in textbooks.

@neutrinoceros
Copy link
Member

radiation density constant is fine

Alright, let's go with that then.

@zingale
Copy link
Member Author

zingale commented Oct 3, 2024

I've updated the name to radiation_density_constant

@neutrinoceros
Copy link
Member

pre-commit.ci autofix

@neutrinoceros neutrinoceros changed the title add radiation constant Fix Stephan-Boltzmann constant accuracy and add radiation constant Oct 4, 2024
@neutrinoceros neutrinoceros merged commit 209fb17 into yt-project:main Oct 4, 2024
9 checks passed
@neutrinoceros
Copy link
Member

Thanks @zingale ! FYI I'm planning to release this into unyt 3.0.4 which I'll hopefully be able to prepare sometimes next week.

@neutrinoceros neutrinoceros added this to the 3.0.4 milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants