support a y indexing different from x

[lmiq]

This change supports, in the _guess_best_legend_position function, different offsets for y and x. Aiming to address #4561.

However, it seems that GR does not support those different offsets, thus the bug must be another.

[t-bltg]

@lmiq, I'm back so we can try to fix this.

What would be conservative atm, and until we know more of the bug from GeoStats docs is to force boundschecking for a (hopefully small) performance penalty, and release a patch version quickly.

We can always remove / rework that later.

Something like:

for i in firstindex(x):max(1, div(min(nsamples, length(x)), 2))
    ii = i + yoffset
    jj = j + yoffset
    if checkbounds(Bool, x, i) && checkbounds(Bool, y, ii)
        inv += inv(1 + weight * d_point(x[i], y[ii], lim, scale))
    end
    if checkbounds(Bool, x, j) && checkbounds(Bool, y, jj)
        inv += inv(1 + weight * d_point(x[j], y[jj], lim, scale))
    end
    j -= 1
end

[lmiq]

It seems that he has a case where lastindex(x) points to an invalid index, which might point to a bug in some array interface definition somewhere else.

[t-bltg]

Wait, isn't that an effect of JuliaArrays/OffsetArrays.jl#77 ?

We index series using a linear index, but they can be n-dimensional 🤔

Of course that assumes the bug occurring in GeoStats uses an OffsetArray.

julia> import OffsetArrays: Origin
julia> oa = zeros(10, 10) |> Origin(-4);
julia> firstindex(oa)
1
julia> firstindex(oa, 1)
-4
julia> firstindex(oa, 2)
-4

Anyway, until we have a mwe we can only speculate.
Let's merge this quickly and release it once ci is green.

Thanks for the reactivity.

[lmiq]

These plots fail, but not at the guessing of the legend position:

julia> x = OffsetArray(rand(3,3), -2, -2);

julia> heatmap(x)
ERROR: BoundsError: attempt to access 3-element OffsetArrays.IdOffsetRange{Int64, Base.OneTo{Int64}} with indices -1:1 at index [2]
Stacktrace:
  [1] throw_boundserror(A::OffsetArrays.IdOffsetRange{Int64, Base.OneTo{Int64}}, I::Tuple{Int64})
    @ Base ./abstractarray.jl:703
  [2] checkbounds
    @ ./abstractarray.jl:668 [inlined]
  [3] getindex
    @ ~/.julia/packages/OffsetArrays/WvkHl/src/axes.jl:224 [inlined]

[t-bltg]

Also, we didn't check length(y) and assumed length(x) == length(y): is that assumption always valid ?

EDIT: that might be it, since the error reported BoundsError: attempt to access 20-element Vector{Float64} at index [100] does not state OffsetArray, but a regular Vector{Float64}.

His error occurs on these types:

x::StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}
y::Vector{Float64}

[codecov]

Codecov Report

Base: 90.94% // Head: 90.95% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (036aaaa) compared to base (2d3b65f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4562   +/-   ##
=======================================
  Coverage   90.94%   90.95%           
=======================================
  Files          40       40           
  Lines        7800     7803    +3     
=======================================
+ Hits         7094     7097    +3     
  Misses        706      706           

Impacted Files	Coverage Δ
src/utils.jl	92.62% <100.00%> (+0.03%)	⬆️

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.

[lmiq]

Also, we didn't check length(y) and assumed length(x) == length(y): is that assumption always valid ?

From what I tested, Plots errors much earlier if that is not true.

[t-bltg]

@lmiq, remove draft & merge now ?

[t-bltg]

Yeah, plot(1:100, collect(Float64, 1:20)) errors, but not in _guess_best_legend_position.
Weird case.
Waiting for the mwe then, I'll try to build GeoStats docs later to narrow the problem.