JuliaAstro / AstroTime.jl

Astronomical time keeping in Julia
https://juliaastro.github.io/AstroTime.jl/stable/
Other
38 stars 10 forks source link

The product of a range and a unit should create a new range, not a vector #65

Open barrettp opened 3 years ago

barrettp commented 3 years ago

The following expression generates a vector:

julia> (1:3)*seconds 3-element Vector{AstroPeriod{AstroTime.Periods.Second, Float64}}: 1.0 seconds 2.0 seconds 3.0 seconds

I argue that it should create a new range:

julia> (1:3)*seconds 1 seconds: 1 seconds: 3 seconds

The reason is that the expression "(1:typemax(Int32))*seconds" is inefficient in terms of time and memory, because it generates a huge vector, instead of a range type.

If you want to generate a vector, then use the dot notation, i.e., "(1:3).*seconds".

This change just requires the addition of two functions, one each for the implied and explicit versions of step.

Also, the expression "(1:typemax(Int64))*seconds" generates an incorrect result, because the maximum value is converted to a float and then back to an integer. This corner case should be handled properly. Int32 works as expected.

mileslucas commented 2 years ago

currently (1:typemax(Int64)) * seconds fails because of the following lines https://github.com/JuliaAstro/AstroTime.jl/blob/a891ffef6fb142d37b8f8836b3ec75da4955606d/src/Periods.jl#L96-L97 https://github.com/JuliaAstro/AstroTime.jl/blob/a891ffef6fb142d37b8f8836b3ec75da4955606d/src/Periods.jl#L49

so regardless of input type, everything goes through a float conversion because all the conversion factors are Float64. I'll open this in a new issue.