Keno / SIUnits.jl

Efficient unit-checked computation
Other
70 stars 26 forks source link

missing one(x) and zero(x) for SIUnit #18

Closed stevengj closed 10 years ago

stevengj commented 10 years ago

one(x) and zero(x) fail for SIUnit types because they default to oftype(x,1) and oftype(x,0), respectively, and there is no convert(Int, x) for SIUnit.

This is problematic for getting e.g. array * Meter to be properly typed (see this mailing-list discussion). It would be nice if these produced an SIQuantity.

tomasaschan commented 10 years ago

Looking at this comment on JuliaLang/Julia#7623, the definitions should probably be something like

one(x::SIQuantity) = SIQuantity(one(x.val))
zero(x::SIQuantity) = SIQuantity(zero(x.val))

With these definitions, e.g. x * one(x) and x * zero(x) will both have the same units as x.

However, that doesn't go all the way to get array * Meter working:

julia> [1.0/Second] * Meter
ERROR: no promotion exists for SIQuantity{SIQuantity{Float64,m,kg,s,A,K,mol,cd},m,kg,s,A,K,mol,cd} and SIQuantity{Float64,m,kg,s,A,K,mol,cd}
 in promote_type at promotion.jl:110
 in .* at array.jl:756
 in * at abstractarray.jl:364

# or even, to make sure the type of the array is OK

julia> SIUnits.SIQuantity{Float64,0,0,-1,0,0,0,0}[1./Second] * Meter
ERROR: no promotion exists for SIQuantity{SIQuantity{Float64,m,kg,s,A,K,mol,cd},m,kg,s,A,K,mol,cd} and SIQuantity{Float64,m,kg,s,A,K,mol,cd}
 in promote_type at promotion.jl:110
 in .* at array.jl:756
 in * at abstractarray.jl:364

Maybe that's something that needs fixing elsewhere? I can make a PR with the new definitions for one and zero if this seems like a good idea.

StefanKarpinski commented 10 years ago

Why not remove the SIQuantity on the right and just return the unit of the underlying value?

tomasaschan commented 10 years ago

@StefanKarpinski I figured that one of the reasons to use one(x) and zero(x) was to get, basically, the multiplicative and additive identities with the same type as x. To be strict, a unitful quantity with unit 1 is not the same thing as a unitless quantity - and therefore, one(x::SIQuantity) should return somthing of type SIQuantity.

Considering this, I think the correct behavior is that

Thus,

one(x::SIQuantity) = SIQuantity(one(x.val))
zero(x::SIQuantity) = zero(x.val) * unit(x)

are probably better definitions.

stevengj commented 10 years ago

@tlycken, I'm still confused. If you want one(x) to be a multiplicative identity (which seems sensible to me), then isn't it dimensionless? Why is it a "unitful" quantity?

tomasaschan commented 10 years ago

@stevengj I make a distinction between "a quantity with no unit information" (e.g. a Float64) and "a quantity with unit 1" (e.g. a SIQuantity{Float64,0,0,0,0,0,0,0}). The latter has a unit, although the unit is 1, which is not the same thing as having no unit information.

I don't know if it will matter much for type stability, performance etc, but I also think it makes more sense that typeof(one(x)) == typeof(x) for any implementation of one, which wouldn't be the case if we just used one(x.val). (It actually isn't the case now either, since unit information is embedded in the type and they will have different units, but at least they're both SIQuantitys...)

stevengj commented 10 years ago

@tlycken, if we make that distinction, then we shouldn't define *(::SIQuantity, ::Float64) at all, because you are interpreting the second argument as a quantity with "no unit information" rather than as a dimensionless quantity, and hence the product has undefined units.

However, given that the current SIQuantity arithmetic rules are defined to interpret Float64 values as dimensionless quantities, it would make sense to me that one(::SIQuantity) return a Float64.

Regarding one(x) returning an SIQuantity, is there a concrete utility of this given that it's still not the same type due to the type parameters?

tomasaschan commented 10 years ago

@stevengj True. Maybe it makes more sense to have one(x::SIQuantity) = one(x.val) and one{T}(::SIQuantity{T,...} = one(T) then.

stevengj commented 10 years ago

On the other hand, an argument for your the one(x::SIQuantity) = one(x.val) definition is that it makes one(x) === x/x for x ≠ 0.

StefanKarpinski commented 10 years ago

That's kind of nice.

tomasaschan commented 10 years ago

Actually, one(x) === x/x would only be true if x is SIQuantity{T<:FloatingPoint, ...}. For any SIQuantity{T<:Integer, ...}, the division in x/x will yield a floating point, so x/x == one(x), but x/x !== one(x) (assuming the difference between == and === is what I think it is).

But you're still successful in convincing me: I'll submit another patch changing this behavior.

StefanKarpinski commented 10 years ago

Yeah, I'm not sure this makes a big difference, but I tend to prefer returning the more basic type where it will work. Not a terribly strong preference in this case though.

tomasaschan commented 10 years ago

I tried formulating a couple of tests that define this behavior, but I seem to be unable to find something that is both general enough to actually define the desired behavior, yet which fails under the current implementation. For example,

@test 3V / 3V == one(3)
@test 3.V / 3.V === one(3.)

both pass with the current version, where one returns an SIQuantity.