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

Fix indexing with Slice #210

Merged
merged 4 commits into from
Feb 26, 2021
Merged

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Feb 25, 2021

Fixes #209

On master and Julia 1.0

julia> using OffsetArrays

julia> r = 1:2:10
1:2:9

julia> r[Base.Slice(Base.OneTo(3))]
ERROR: StackOverflowError:
Stacktrace:
 [1] getindex(::StepRange{Int64,Int64}, ::Base.Slice{Base.OneTo{Int64}}) at /home/jishnu/Dropbox/JuliaPackages/OffsetArrays.jl/src/OffsetArrays.jl:352 (repeats 80000 times)

After this PR:

julia> using OffsetArrays

julia> r = 1:2:10
1:2:9

julia> r[Base.Slice(Base.OneTo(3))]
1:2:5 with indices 1:3

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #210 (36fbb3f) into master (23de436) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #210   +/-   ##
=======================================
  Coverage   98.28%   98.28%           
=======================================
  Files           5        5           
  Lines         291      291           
=======================================
  Hits          286      286           
  Misses          5        5           
Impacted Files Coverage Δ
src/OffsetArrays.jl 97.93% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23de436...36fbb3f. Read the comment docs.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up having to try the tests to understand this, but now that I do the one thing I'd request is adding

r2 = IdentityUnitRange(2:10)

and

r2 = IdOffsetRange(Base.OneTo(9), 1)

to the tests. My reading is that it should still work, but given that no_offset_view performs "real work" in these cases that's a bit different from what's in the test coverage, I think we should check.

@jishnub
Copy link
Member Author

jishnub commented Feb 26, 2021

I've added tests for IdOffsetRange(Base.OneTo(9), offset). Some tests for r2 = IdentityUnitRange(2:10) were already there, but now I've added some other r1 types to make the tests for r1[r2] more robust.

@jishnub jishnub merged commit 516df47 into JuliaArrays:master Feb 26, 2021
@jishnub jishnub deleted the fixindexingslice branch February 26, 2021 16:49
@rikhuijzer
Copy link

rikhuijzer commented Feb 26, 2021

Thanks a lot for fixing this so quickly 😄 Could you also tag the patch/call JuliaRegistries register? CI is still failing at https://github.com/TuringLang/MCMCChains.jl.

@rikhuijzer
Copy link

@jishnub

@timholy
Copy link
Member

timholy commented Feb 26, 2021

JuliaRegistries/General#30890

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.

Latest release causes StackOverflowError
3 participants