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 Earth Mass and Jupiter Mass as units #3907

Merged
merged 7 commits into from Oct 2, 2015

Conversation

keflavich
Copy link
Contributor

I have had need to use Earth Mass and Jupiter Mass as units, not just
constants. One can easily do this per-session, but I see no reason not to
include them in astropy, since they're often quoted as units for brown dwarf
and exoplanet studies.

The per-session approach:

M_earth = u.Unit('mearth', constants.M_earth)
M_jup = u.Unit('Mjup', constants.M_jup)

I'd like to do the same for a Lunar mass (which isn't in constants either), but
I can't find a reliable reference for the mass including an uncertainty. I
don't have Allen's Astrophysical Quantities - could someone with access to that
book put up the numbers?

@bsipocz
Copy link
Member

bsipocz commented Jul 1, 2015

👍 to have them as units

def_unit(['earthMass', 'M_earth', 'Mearth'], _si.M_earth.value * si.kg, namespace=_ns,
prefixes=True, doc="Earth mass",
# LaTeX earth symbol requires wasysym
format={'latex': r'M_{\oplus}', 'unicode': 'M'})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a glitch with the earth symbol here. It looks like it's uf728 (which is undefined), but you probably meant u1f728 (ASTRONOMICAL SYMBOL FOR VERDIGRIS). However, I'd recommend using u2295 (CIRCLED PLUS) instead, since I think it's a lot more widely available in various fonts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, fixed

Copy link
Member

Choose a reason for hiding this comment

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

This is still showing incorrectly for me. Did you push the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

@embray
Copy link
Member

embray commented Jul 1, 2015

Seems like a no-brainer 👍

@embray
Copy link
Member

embray commented Jul 1, 2015

Several tests appear to be failing due to the addition of new units (this includes, for example, a doctest for the units listing page). So please try to resolve those locally first.

_si.M_jup.value * si.kg, namespace=_ns,
prefixes=True, doc="Jupiter mass",
# LaTeX jupiter symbol requires wasysym
format={'latex': r'M_{Jup}', 'unicode': 'M♃'})
Copy link
Contributor

Choose a reason for hiding this comment

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

The latex form should be r'M_{\rm Jup}'. Actually, should it be r'M_{\rm J}'? I think that is used much more commonly (even in the IAU definition in [1])

[1] http://asa.usno.navy.mil/static/files/2014/Astronomical_Constants_2014.pdf
(note this also defines the moon-to-earth mass ratio).
Also http://maia.usno.navy.mil/NSFA/NSFA_cbe.html#MSMJ2009

@keflavich
Copy link
Contributor Author

@embray I think the tests passed, there was just a timeout error on one.

@embray embray added this to the v1.1.0 milestone Jul 2, 2015
@embray
Copy link
Member

embray commented Jul 2, 2015

Just needs a changelog entry under v1.1 / New Features / units. You can use [skip ci] in the commit message to prevent another build from running.

@astrofrog
Copy link
Member

@keflavich - can you rebase today so we can include in 1.1?

@keflavich
Copy link
Contributor Author

@astrofrog - done. I had to do one nontrivial merge that might be worth double-checking:
https://github.com/astropy/astropy/pull/3907/files#diff-5450b535e19a0a42df573bdfdb95e22aR392

Also, I think the [skip ci] message may have caused the rebase not to undergo ci, which is bad. I'll rebase again and modify that message...

@astrofrog
Copy link
Member

@keflavich - there are some PEP8 failures, can you fix? Thanks!

[u.M_e, u.M_p, u.g, u.kg, u.solMass, u.t, u.u,
imperial.oz, imperial.lb, imperial.st, imperial.ton])
[u.M_e, u.M_p, u.g, u.kg, u.solMass, u.t, u.u, u.M_earth,
u.M_jup, imperial.oz, imperial.lb, imperial.st, imperial.ton])
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is messed up here (there's a tab)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - my mergetool doesn't follow my space-only standards.

@@ -198,6 +198,7 @@ New Features
- Added furlong to imperial units. [#3529]
- Added mil to imperial units. [#3716]
- Added stone to imperial units. [#4192]
- Added Earth Mass (``Mearth``) and Jupiter mass (``M_jup``) to units [#3907]
Copy link
Member

Choose a reason for hiding this comment

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

Should be M_earth - might as well make it consistent with M_jup (but include [ci skip] for this)

@astrofrog
Copy link
Member

Thanks!

astrofrog added a commit that referenced this pull request Oct 2, 2015
Add Earth Mass and Jupiter Mass as units
@astrofrog astrofrog merged commit 9706070 into astropy:master Oct 2, 2015
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