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

convert for Periods is a pun? #16109

Closed
TotalVerb opened this issue Apr 28, 2016 · 7 comments
Closed

convert for Periods is a pun? #16109

TotalVerb opened this issue Apr 28, 2016 · 7 comments
Labels
dates Dates, times, and the Dates stdlib module

Comments

@TotalVerb
Copy link
Contributor

TotalVerb commented Apr 28, 2016

Currently, convert is used to extract the magnitude of a quantity, or alternatively to give units to a dimensionless quantity. This happens in the Dates code in Base, and the behaviour has inspired similar behaviour in SIUnits.jl and Unitful.jl.

In Base, convert is generally used to convert between values of different types, but with same meaning. Since 1 and 1 second do not represent the same quantity, the use of convert to switch between them is a pun. In addition, it can lead to surprising behaviour. Note:

julia> convert(Base.Dates.Second, 60) 
60 seconds

julia> convert(Base.Dates.Minute, ans)
1 minute

julia> convert(Int, ans)
1

So we have converted an Int back to itself, but the conversion was lossy!

@quinnj
Copy link
Member

quinnj commented Apr 28, 2016

Hmm.....I'm not sure I see the actual problem; I think we're safely within the general meaning of convert here. I'm also wondering what else you'd expect to see here? It's not immediately obvious what other path we would take.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Apr 28, 2016

The problem is that convert is currently dimensionally incorrect.

Here is some food for thought. If I had a type Radians that represented a quantity multiplied by 2π:

immutable Radian
    val::Float64
end

convert(::Type{Radian}, x::Float64) = ???

then what should convert do here? I would argue that the correct conversion is Radian(x / 2π). This preserves the mathematical meaning of a Radian. Since radians are dimensionless, this conversion cannot lead to any trouble. This kind of conversion has the same meaning as the conversion between Second and Minute, and is what I personally expected of convert; it should preserve meaning, not necessarily the magnitude of the value internally stored.

But the Period conversion is not based at all on the mathematical meaning of a Period, but rather based off the number of units of measure. If we generalized that to the Radian type, we would end up with the conversion Radian(x) instead.

Because these two different approaches lead to a different result, it would seem that they are actually two distinct operations, and convert is therefore currently a pun. The operation of putting units on things should perhaps be given a new name, and not as an additional method on convert?


Mathematica uses QuantityMagnitude to remove a unit, Quantity to add a unit, and UnitConvert to convert a quantity to a unit of same dimension. I think convert is best suited to UnitConvert, and the other functionality should have a different name.

@tkelman tkelman added the dates Dates, times, and the Dates stdlib module label Apr 29, 2016
@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

related: #15394

@jiahao
Copy link
Member

jiahao commented Apr 29, 2016

Duplicate of #10451

@toivoh
Copy link
Contributor

toivoh commented Apr 29, 2016

I agree with @TotalVerb that we need to introduce a mechanism for conversions between incommensurable units that does not rely on convert.

One way might be to have a measure function such that e.g.

measure(s, 5s) = 5
measure(m, 4m/s) = 4/s

eg you specify a unit to be divided out from the quantity.

@nalimilan
Copy link
Member

So basically to be consistent convert should always return the number of seconds or milliseconds. This is what @ivarne suggested in the other issue.

@jiahao
Copy link
Member

jiahao commented Apr 29, 2016

Closing as duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

No branches or pull requests

6 participants