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

Astropy updated constants #4956

Closed
wants to merge 7 commits into from

Conversation

pmaxted
Copy link

@pmaxted pmaxted commented May 19, 2016

I have updated the the solar and planetary constants in constants/si.py to be consistent with 2015 IAU resolution B3. The constants are now "nominal values" or "conversion factors".

To convert the mass parameters like "GM_Sun" to actual masses like M_Sun, a value of G must be chosen. The user would expect that this would be the CODATA 2014 value mentioned in Resolution B3, so I have updated that value also. For consistency, I updated all the fundamental constants to CODATA 2014 values.

I have changed some of the variable names so that it is clear that they are polar/equatorial radii (e.g., R_earth -> Re_Earth or Rp_Earth). I have also changed some other variable names like R_jup to R_Jup. This will may require some users to change their code, but this should also mean that they can check that these new constants are really the numbers that they want.

The following files needed changing to update to the new constant names: units/astrophys.py, units/cds.py, coordinates/tests/accuracy/test_ecliptic.py, analytic_functions/tests/test_blackbody.py

I updated an example in units/core.py docstring to be consistent with the new output from u.m.find_equivalent_units()

The following tests required updated "hard-coded" test values to be consistent with CODATA 2014:

test_critical_density (test_cosmology.py)
9.31000324385361e-30 -> 9.30966845602090e-30
[2.70362491e-29, 5.53758986e-28] -> [2.70352772e-29, 5.53739080e-28]

(and delete the repetition of this last line)

test_doppler_energy_0 (test_equivalencies.py)
q1 = 0.000434286445543 * u.eV -> q1 = 0.000434286461222 *u.eV

test_brightness_temperature
tb = 7.05258885885 * u.K -> tb = 7.05259028913 * u.K

@astrofrog
Copy link
Member

Thanks for the PR!

There are two other similar PRs: #4397 and #4109, and we need to make a decision on how to deal with versioning of constants.

@@ -204,6 +218,8 @@ API changes
- ``astropy.conftest.py``

- ``astropy.constants``
- Changed constant names for solar/planetary conversion factors
(R_sun -> R_Sun, R_earth -> Re_Earth, etc.)
Copy link
Member

Choose a reason for hiding this comment

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

A big 👎 for this name change from me.

  • They need to be at least deprecated first before removal.
  • doing such an API change just to change the case is totally unneccessary, and leaves inconsistency in with the rest of the code.
  • I'm pretty sure most of us working with planets want to keep using R_earth and R_jup as is, without the extra complications of whether it is polar or equatorial.

Copy link
Contributor

Choose a reason for hiding this comment

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

Strongly second the concerns about the name change, except for the 3rd point:
the existing R_earth, R_jup already refer to the equatorial radii. One could therefore add the polar values and link the old name to the equatorial one:
R_earth = Re_earth etc.
Although it could also be argued for defining a volume-equal mean radius
R_earth = (Re_earth**2 * Rp_earth)**(1./3)
but that would again create inconsistencies with many people's current usage.

@bsipocz
Copy link
Member

bsipocz commented May 19, 2016

@astrofrog - another related issue that started the discussion: #3843

@mhvk
Copy link
Contributor

mhvk commented May 19, 2016

I just created a new issue for the version control, which I think we indeed need to address first, as it is holding up so many PRs now. See #4958.

@mhvk mhvk added this to the v1.3.0 milestone May 19, 2016
@pllim
Copy link
Member

pllim commented Nov 28, 2016

I see that the tag is 1.3 but given the ongoing discussions and Friday's feature freeze, it seems unrealistic.

@mhvk mhvk modified the milestones: Future, v1.3.0 Nov 29, 2016
@mhvk
Copy link
Contributor

mhvk commented Nov 29, 2016

@pllim - agreed - I've removed it.

@pllim
Copy link
Member

pllim commented May 9, 2017

This does not appear to be the converging solution discussed in #4958, and no to name change.

@pllim pllim closed this May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants