JuliaArrays / OffsetArrays.jl

Fortran-like arrays with arbitrary, zero or negative starting indices.
Other
198 stars 40 forks source link

Should we export IdOffsetRange? #267

Open timholy opened 3 years ago

timholy commented 3 years ago

266 contained a use case where having a real offset range seemed necessary. We have one, IdOffsetRange, which is not exported. Should we export it? What kind of trouble is that likely to cause? And if we export it, should we rename it? It's "id" only under certain circumstances.

jishnub commented 3 years ago

The devdocs advise against exporting such ranges to avoid conflicts with other packages that may define their own types (although I wonder if you wrote that yourself?). Perhaps it'll be better to go with your other idea and export a function offsetarray that may return an appropriate type?

timholy commented 3 years ago

The me from back then clearly knew what he was talking about. We should listen to him :laughing:

Tokazama commented 3 years ago

Could we instead have something like...

offset(x::AbstractUnitRange, o::Integer) = IdOffsetRange(x, o)
offset(x::AbstractArray{T,N}, o::Tuple{Vararg{Integer,N}}) where {T,N} = OffsetArray(x, o)
offset(o::Union{Integer,Tuple{Vararg{Integer}}) = Base.Fix2(offset, o)

...which would allow constructing the offset types or passing along offsets for construction later.

jishnub commented 3 years ago

That's an interesting idea, and I can see something like this being useful.

julia> zerobasedinds = offset((-1,-1));

julia> A = zerobasedinds(zeros(3,3));

julia> A = zeros(3,3); B = zerobasedinds(A);

julia> B[0,0] = 3;

julia> A
3×3 Matrix{Float64}:
 3.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

Although in this case it is equally possible to use

julia> zerobasedinds2(A) = OffsetArray(A, OffsetArrays.Origin(0));

julia> A = zeros(3,3); B = zerobasedinds2(A);

julia> B[0,0] = 4;

julia> A
3×3 Matrix{Float64}:
 4.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

From what I understand, the suggestion is not specific to offsets (which might be considered an internal detail), but rather to have a Fix2 version of the proposed offsetarray function that may accept either axes or Origin as the second argument.

jishnub commented 3 years ago

On a related note, something like this seems like a useful pattern to temporarily change the axes of an array:

julia> struct WithOrigin{T <: OffsetArrays.Origin}
       o :: T
       end

julia> WithOrigin(t::Union{Integer, Tuple{Integer, Vararg{Integer}}}) = WithOrigin(OffsetArrays.Origin(t))
WithOrigin

julia> (w::WithOrigin)(A::AbstractArray) = OffsetArray(A, w.o);

julia> (w::WithOrigin)(f, A::AbstractArray) = f(w(A))

julia> A = zeros(3,3);

julia> WithOrigin(0)(A) do B
       B[0,0] = 4
       end;

julia> A
3×3 Matrix{Float64}:
 4.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

This is somewhat limited at present, as Origin may not be combined with axes in the constructor. Perhaps it might make sense to support a combination of Origin and axes in the OffsetArray constructor, as one might want to specify the origin along certain dimensions and axes along others (eg. a : to not apply an offset).

johnnychen94 commented 3 years ago

On a related note, something like this seems like a useful pattern to temporarily change the axes of an array:

This idea is quite similar to https://github.com/JuliaArrays/OffsetArrays.jl/pull/248