-
Notifications
You must be signed in to change notification settings - Fork 24
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
Take advantage of UnitTuple to store type info #78
base: master
Are you sure you want to change the base?
Conversation
Indeed, this package was written for 0.2, where that wan't possible yet. In particular #5, the earliest still open issue in this package. |
Oh, I didn't see that. Yea, that makes sense. Do you still want to support 0.2 with this package, or could something like this be merged? |
No, 0.2 is obsolete at this point
|
Ok. I noticed the latex output is broken on new version, and that the coverage tool is failing. Are you fine with change otherwise? |
@@ -7,15 +7,28 @@ module SIUnits | |||
import Base: ==, +, -, *, /, .+, .-, .*, ./, //, ^ | |||
import Base: promote_rule, promote_type, convert, show, mod | |||
|
|||
immutable SIQuantity{T<:Number,m,kg,s,A,K,mol,cd,rad,sr} <: Number | |||
typealias UnitTuple NTuple{9,Int64} |
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.
Is Int64
really the correct thing here? Will this still be able to represent units like s^(1/2)
?
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.
I haven't tried it with rational exponents, but there doesn't seem to be
anything that prevents it. Does it currently work properly with rational
exponents?
On Thu, Nov 19, 2015 at 11:22 PM Tomas Lycken [email protected]
wrote:
In src/SIUnits.jl
#78 (comment):@@ -7,15 +7,28 @@ module SIUnits
import Base: ==, +, -, , /, .+, .-, ., ./, //, ^
import Base: promote_rule, promote_type, convert, show, mod
- immutable SIQuantity{T<:Number,m,kg,s,A,K,mol,cd,rad,sr} <: Number
- typealias UnitTuple NTuple{9,Int64}
Is Int64 really the correct thing here? Will this still be able to
represent units like s^(1/2)?—
Reply to this email directly or view it on GitHub
https://github.com/Keno/SIUnits.jl/pull/78/files#r45441519.
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.
Actually, I was quite sure it should work (in fact, IIRC the move from ints-and-types to (values-of-any-bitstypes)-and-types as valid type parameters was made in base Julia with e.g. s^(1/2)
as the main use case), but when I try now, it doesn't:
julia> using SIUnits; Second^(1/2)
1.0
Don't know if this is a regression or if it never worked, though.
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.
There is a test case that checks that sqrt(1s)
throws an error, so I assume that hasn't worked for a while.
But with some minor changes, I was able to switch it to
typeaias UnitTuple NTuple{9,Rational{Int64}}
It seems to work fine with some basic testing, and I would think this would be superset of the current functionality of this PR. Not sure if this is what you had in mind, or something more generic like
typeaias UnitTuple{N<:Number} NTuple{9,N}
I would guess that this would require a decent amount of work to get working
I tried recreating this package as an exercise to understand the type system better. I found that using UnitTuple directly in SIUnit and SIQuantity simplified things quite a bit. I updated this package to work that way as well. Its a big change, but the tests are passing for me, so hopefully I fixed everything that needs to be fixed. I have a few other changes that simplify a few things, but this was the biggest change.