-
Notifications
You must be signed in to change notification settings - Fork 49
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
Various new non-metric (and one metric) units #441
Conversation
LGTM. If you can, it would help future readers to add comments with e.g. links to where the exact numerical constants are coming from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the missing suffix I found, LGTM !
@@ -186,7 +207,11 @@ | |||
("atm", (pascal_per_atm, dimensions.pressure, 0.0, r"\rm{atm}", False)), | |||
("hp", (watt_per_horsepower, dimensions.power, 0.0, r"\rm{hp}", False)), | |||
("oz", (kg_per_pound / 16.0, dimensions.mass, 0.0, r"\rm{oz}", False)), | |||
("ton", (kg_per_pound * 2000.0, dimensions.mass, 0.0, r"\rm{ton}", False)), | |||
("ton", (kg_per_pound * 2000.0, dimensions.mass, 0.0, r"\rm{US ton}", False)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
("ton", (kg_per_pound * 2000.0, dimensions.mass, 0.0, r"\rm{US ton}", False)), | |
("ton_US", (kg_per_pound * 2000.0, dimensions.mass, 0.0, r"\rm{US ton}", False)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ton
is already here and we weren't thinking about US vs UK when we implemented it (mistake). So I'm not sure how to handle it without breaking back-compat, other than making ton_US
the unit and ton
the alias (which I guess works).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, I didn't realise it was already implemented ! nevermind then
This PR adds a number of new non-metric and one new metric unit to the unit lookup table. Inspired originally by @ring-michael