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 unit of year #288

Merged
merged 3 commits into from
Feb 21, 2020
Merged

Add unit of year #288

merged 3 commits into from
Feb 21, 2020

Conversation

briochemc
Copy link
Contributor

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?)

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-io
Copy link

codecov-io commented Dec 10, 2019

Codecov Report

Merging #288 into master will increase coverage by 3.91%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/pkgdefaults.jl 33.92% <ø> (+13.52%) ⬆️
src/conversion.jl 84.21% <0%> (-0.41%) ⬇️
src/user.jl 94.44% <0%> (-0.13%) ⬇️
src/Unitful.jl 100% <0%> (ø) ⬆️
src/dimensions.jl 94.59% <0%> (+0.15%) ⬆️
src/units.jl 86.56% <0%> (+0.3%) ⬆️
src/fastmath.jl 82.53% <0%> (+2.53%) ⬆️
src/logarithm.jl 66.81% <0%> (+2.72%) ⬆️
src/quantities.jl 94.22% <0%> (+4.86%) ⬆️
src/range.jl 83.33% <0%> (+5.28%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51a8650...1edf03b. Read the comment docs.

Add tests for conversion between time units: s ⟷ min ⟷ hr ⟷ d ⟷ yr
@briochemc
Copy link
Contributor Author

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.
@cstjean
Copy link
Contributor

cstjean commented Dec 10, 2019

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 1year != Year(1).

Those are minor points though, I wouldn't mind.

@briochemc
Copy link
Contributor Author

wouldn't it make sense to keep it application-specific, since different domains might have different requirements?

My opinion is that it is still better for (most?) Unitful users to have a default u"yr" available. If a specific application really needs something different then I believe it should precisely care about making its own precise definition for it, no?

@ajkeller34
Copy link
Collaborator

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.

@ajkeller34 ajkeller34 closed this Feb 9, 2020
@ajkeller34 ajkeller34 reopened this Feb 9, 2020
@ajkeller34 ajkeller34 merged commit 7526dc7 into PainterQubits:master Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants