-
Notifications
You must be signed in to change notification settings - Fork 112
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 unit of year #288
Add unit of year #288
Conversation
I think that expressing quantities in units of years is so ubiquitous in science that it would be nice to have it in the default Unitful. FWIW I used 365.25 days = 31557600 seconds for the conversion like in UnitfulAstro.jl. There may have been previous discussions about this but I could not find them. (Maybe they disappeared when Unitful moved to PainterQubits?)
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
==========================================
+ Coverage 78.13% 82.05% +3.91%
==========================================
Files 15 15
Lines 1075 1198 +123
==========================================
+ Hits 840 983 +143
+ Misses 235 215 -20
Continue to review full report at Codecov.
|
Add tests for conversion between time units: s ⟷ min ⟷ hr ⟷ d ⟷ yr
I realize now that one might want to change the example exteding Unitful because it extends it with "yr". |
This is a just suggestion where I changed the example of extending Unitful to use "year" instead of "yr" (and 365 days instead of 365.25) such that it is distinct from the "yr" unit that this PR adds to default units.
I also defined year to be 365.25 days, but wouldn't it make sense to keep it application-specific, since different domains might have different requirements? Furthermore, if #125 is still planned, we would have Those are minor points though, I wouldn't mind. |
My opinion is that it is still better for (most?) Unitful users to have a default |
I think I agree with these changes. For calculations with physical quantities it seems like the most generically useful definition is the one in this PR. Other definitions could shadow if needed. Will close and reopen to retrigger CI. |
I think that expressing quantities in units of years is so ubiquitous in science that it would be nice to have it in the default Unitful.
FWIW I used 365.25 days = 31557600 seconds for the conversion like in UnitfulAstro.jl.
There may have been previous discussions about this but I could not find them. (Maybe they disappeared when Unitful moved to PainterQubits?)