eachindex call incorrectly assumes that the returned index is IndexLinear

[bkamins]

I am not sure if other calls to eachindex in Plots.jl have the same problem. The fix should resolve https://discourse.julialang.org/t/plots-borking-on-sentinelarrays-produced-by-csv-read/89505, but maybe there are other cases in the code when the same issue would be raised (I have checked the eachindex calls, but often I was not sure if the type on which it is called returns IndexLinear by default or not).

[bkamins]

This fix assumes that x in the code is guaranteed to be AbstractArray. I am not sure if it is guaranteed (I would assume it is, but I am not sure as from the signature it is not clear), so someone knowing Plots.jl should confirm this. Thank you!

[bkamins]

Also, more generally - it would be good to confirm that _cycle works correctly with OffsetArrays.jl (I am not using it, but this might be another issue of a similar nature).

[codecov]

Codecov Report

Base: 90.51% // Head: 90.51% // No change to project coverage 👍

Coverage data is based on head (9c2e119) compared to base (d37774f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4479   +/-   ##
=======================================
  Coverage   90.51%   90.51%           
=======================================
  Files          40       40           
  Lines        7635     7635           
=======================================
  Hits         6911     6911           
  Misses        724      724           

Impacted Files	Coverage Δ
src/backends/gr.jl	92.61% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bkamins]

x-ref JuliaLang/julia#47389

[t-bltg]

Thanks, git blame shows that this isn't due to a recent change.

Some tests (and maybe OffsetArrays ones) should be added to https://github.com/JuliaPlots/Plots.jl/blob/master/test/test_utils.jl, so that we don't run into this again.

@bkamins, you don't mind me pushing in your patch-1 branch ?

[bkamins]

you don't mind me pushing in your patch-1 branch ?

sure - it will be much faster for someone knowing the internals to make these things correct than me. Thank you!

[t-bltg]

Thanks, I'm trying to understand the failing example to cook up a test.
Do you know which package wraps data into SentinelArrays ? Is it DataFrames ?

[bkamins]

No, CSV.jl creates SentinelArrays. I recommend you use SentinelArrays.jl directly for tests.

A reproducer is:

julia> using Plots, SentinelArrays

julia> scatter(ChainedVector([[1, 2], [3, 4]]), 1:4)
Error showing value of type Plots.Plot{Plots.GRBackend}:
ERROR: MethodError: no method matching _cycle(::ChainedVector{Float64, Vector{Float64}}, ::SentinelArrays.ChainedVectorIndex{Vector{Float64}})

Another reproducer is:

julia> using OffsetArrays

julia> scatter(OffsetArray(1:4, -2), 1:4)

which produces the following plot (clearly incorrect due to index alignment and _cycle design):

[t-bltg]

Thanks for the examples.
It is hard to say if the OffsetArrays example is "wrong".
I couldn't find the justification for this design, so a safe approach here would be to only fix the SentinelArrays bug ...

[bkamins]

Yes - that is why this PR only fixes SentinelArrays.jl issue. For OffsetArrays.jl a decision would need to be made what is the desired behavior.