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

Should we export IdOffsetRange? #267

Open
timholy opened this issue Sep 17, 2021 · 6 comments
Open

Should we export IdOffsetRange? #267

timholy opened this issue Sep 17, 2021 · 6 comments

Comments

@timholy
Copy link
Member

timholy commented Sep 17, 2021

#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
Copy link
Member

jishnub commented Sep 17, 2021

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
Copy link
Member Author

timholy commented Sep 17, 2021

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

@Tokazama
Copy link
Member

Tokazama commented Oct 18, 2021

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
Copy link
Member

jishnub commented Oct 18, 2021

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
Copy link
Member

jishnub commented Oct 18, 2021

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
Copy link
Member

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 #248

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

No branches or pull requests

4 participants