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

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

Open
barrettp opened this issue Aug 26, 2021 · 1 comment · May be fixed by #67
Open

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

barrettp opened this issue Aug 26, 2021 · 1 comment · May be fixed by #67

Comments

@barrettp
Copy link

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 mileslucas linked a pull request Oct 26, 2021 that will close this issue
@mileslucas
Copy link
Member

currently (1:typemax(Int64)) * seconds fails because of the following lines

seconds = dt * factor(unit)
int_seconds = floor(Int64, seconds)

factor(::Second) = 1.0

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants