search

JuliaAstro / AstroTime.jl

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

Incorrect value when using TT time in a range expression #60

Closed barrettp closed 3 years ago

barrettp commented 3 years ago

The following range expression produces an incorrect value when using TT time.

TTEpoch("2021-05-09T07:01:10"):TTEpoch("2021-05-09T08:02:00") 2021-05-09T07:01:10.000 TT:86400.0 seconds:2021-05-09T07:01:10.000 TT

giordano commented 3 years ago

It probably depends on what you expect it to do. Would you expect both endpoints to be included? That's not what the lower:step:upper syntax does: the lower endpoint is always included, but upper may or may not be included, depending on whether upper - lower is an integer multiple of step. If you want to include both endpoints you should look into range.

barrettp commented 3 years ago

However, the full version of the function returns the result that I expected. The short version apparently assumes units of days, not seconds.

TTEpoch("2021-05-09T07:01:10"):1seconds:TTEpoch("2021-05-09T07:01:20") 2021-05-09T07:01:10.000 TT:1 second:2021-05-09T07:01:20.000 TT

helgee commented 3 years ago

I'd say that the current code works as intended, see here: https://github.com/JuliaAstro/AstroTime.jl/blob/8e0936b59b420dc104f232ae50e12b8857e32979/src/Epochs/ranges.jl#L1

The question is whether the intention is the correct one 😉 It would make sense to change the default unit to seconds, i.e. an SI unit. I am not sure why I chose days in the first place. Probably because I was used to working with Julian Day numbers...

barrettp commented 3 years ago

Yes, the code works as intended. The issue is: when you enter a time accurate to the second, I think you would expect the result to be accurate to the second and that was not the case. I would error on the side of greater precision and let the user decide otherwise.

giordano commented 3 years ago

It's a fair observation, but I think we don't really have an easy way to tell what precision the user used. Defaulting to one second probably makes sense, but someone may expect even greater granularity. As a general remark, when using the : syntax I recommend explicitly using the desired step to avoid any surprise (or problems when the default value is changed).

barrettp commented 3 years ago

Yes, I had considered the precision argument. My recommendation then is to modify the help document to emphasize using the long form of range operator with the step size, instead of the short form. This can be used to inform the user that the default step size is in days.