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

Missing operations for LinSpace. Fix #11049 #11233

Merged
merged 1 commit into from
May 22, 2015

Conversation

yuyichao
Copy link
Contributor

Make it as closed to FloatRange as possible. Hopefully I'm using the right algorithm....

julia> linspace(0, 10, 20)
linspace(0.0,10.0,20)

julia> 0 + linspace(0, 10, 20) + 0
linspace(0.0,10.0,20)

julia> 0 - linspace(0, 10, 20) - 0
linspace(0.0,-10.0,20)

julia> 1 * linspace(0, 10, 20) * 1
linspace(0.0,10.0,20)

julia> linspace(0, 10, 20) / 1
linspace(0.0,10.0,20)

julia> -linspace(0, 10, 20)
linspace(-0.0,-10.0,20)

The divided by 0 case is a little different from FloatRange

julia> linspace(0, 10, 21) / 0
linspace(NaN,Inf,21)

julia> 0.0:0.5:10.0 / 0
0.0:0.5:Inf

julia> [linspace(0, 10, 21) / 0;]
21-element Array{Float64,1}:
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN
 NaN

julia> [0.0:0.5:10.0 / 0;]
ERROR: InexactError()
 in vcat at ./range.jl:650

IMHO both are wrong because

julia> [linspace(0, 10, 21);] / 0
21-element Array{Float64,1}:
 NaN
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf

julia> [0.0:0.5:10.0;] / 0
21-element Array{Float64,1}:
 NaN
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf
 Inf

But I'm waiting to see what ppl think...

@yuyichao
Copy link
Contributor Author

Hmmm? The CI fails far before it reaches anything I changed when trying to get a version number??

@yuyichao
Copy link
Contributor Author

Actually I think I need to handle the case when length() is 0 or 1...

@yuyichao yuyichao changed the title Missing operations for LinSpace. Fix #11049 (WIP) Missing operations for LinSpace. Fix #11049 May 12, 2015
@yuyichao yuyichao changed the title (WIP) Missing operations for LinSpace. Fix #11049 Missing operations for LinSpace. Fix #11049 May 15, 2015
@yuyichao
Copy link
Contributor Author

  • Fixed some type and the getindex for ranges.
  • Added more tests.
  • Rebased on latest master
  • (Edit:) Also test the return type of getindex

@StefanKarpinski

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

LGTM. I'm a little confused why both LinSpace and FloatRange exist now and aren't quite the same thing, but if we're going to have both, may as well have things tested and functioning.

@IainNZ
Copy link
Member

IainNZ commented May 16, 2015

Wooah yeah, I missed when LinSpace was a type, thats kinda funky

@yuyichao
Copy link
Contributor Author

Well, yeah, that's the same question I asked in #11049.

It seems that they are using slightly different algorithms so in corner cases they may produce slightly different results (and IMHO the linspace one is better for small steps).

@timholy
Copy link
Member

timholy commented May 16, 2015

Background is #9666, #9637, but I didn't follow this in detail. @StefanKarpinski, since this is your domain it would be best if you reviewed this.

@timholy
Copy link
Member

timholy commented May 16, 2015

(But it all LGTM.)

@yuyichao
Copy link
Contributor Author

@StefanKarpinski

@StefanKarpinski
Copy link
Member

Yeah, seems right. Sorry about missing these. I'm still not fully convinced about this change, but since we currently have it, may as well have the more complete version.

StefanKarpinski added a commit that referenced this pull request May 22, 2015
Missing operations for LinSpace. Fix #11049
@StefanKarpinski StefanKarpinski merged commit 522a5a4 into JuliaLang:master May 22, 2015
@yuyichao yuyichao deleted the linspace-op branch May 22, 2015 22:21
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 this pull request may close these issues.

5 participants