Skip to content

EachRow/EachCol types (see #32310)#663

Draft
simonbyrne wants to merge 1 commit into
masterfrom
sb/eachrow
Draft

EachRow/EachCol types (see #32310)#663
simonbyrne wants to merge 1 commit into
masterfrom
sb/eachrow

Conversation

@simonbyrne

@simonbyrne simonbyrne commented Oct 8, 2019

Copy link
Copy Markdown
Member

Obviously still needs JuliaLang/julia#32310 to be merged, but this is just to show we can monkey-patch the aliases to support older versions of Julia.

cc: @nalimilan

Comment thread src/Compat.jl

# https://github.com/JuliaLang/julia/pull/32310
if v"0.7.0" <= VERSION
const EachRow{A,I} = Union{Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}, Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const EachRow{A,I} = Union{Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}, Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}
const EachRow{A,I} = Union{Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}, Base.Generator{I,(typeof(eachrow(ones(Int32,2,2)).f).name.wrapper){A}}}

@oschulz

oschulz commented May 12, 2022

Copy link
Copy Markdown

Could we also define Compat.AbstractSlices as part of this to "backport" Base.AbstractSlices (see JuliaLang/julia#32310 (comment))? This will allow packages like JuliennedArrays.jl, ArraysOfArrays.jl to extend that type without depending on non-LTS Julia versions.

@oschulz

oschulz commented May 12, 2022

Copy link
Copy Markdown

We could also add

const EachSlice{A,I} = Base.Generator{I,<:(typeof(eachslice(ones(Int32,2,2), dims = 1).f).name.wrapper){A}}
Base.parent(x::EachSlice) = x.f.A

to add support for "legacy" eachslice, in addition to "legacy" eachrow and eachcol. Seems to work fine:

julia> parent(eachslice(rand(3,4,5,6), dims = 3))
3×4×5×6 Array{Float64, 4}:
#...

@martinholters

Copy link
Copy Markdown
Member

With JuliaLang/julia#32310 finally merged, this could now be picked up again if there is still interest.

@oschulz

oschulz commented Jun 10, 2022

Copy link
Copy Markdown

I think we should definitely do this. Ideally with AbstractSlices and EachSlice as well?

@nalimilan

Copy link
Copy Markdown
Member

Adding AbstractSlices in the spirit of this PR isn't possible AFAICT as on Julia < 1.9 eachrow and eachcol return Generator objects, which cannot be made to inherit from AbstractSlices. What can be done is adding unexported eachrow, eachcol and eachslice functions that would be copies of what has been added at JuliaLang/julia#32310, i.e returning EachSlice <: AbstractSlices objects.

Probably the best would be to implement both approaches. The former is useful for packages that want to use a special method when users call f(eachcol(x)) on any Julia version. The latter is useful if they want to dispatch on AbstractSlices, and/or call Compat.eachslice themselves.

@oschulz

oschulz commented Jun 12, 2022

Copy link
Copy Markdown

Adding AbstractSlices in the spirit of this PR isn't possible AFAICT

Ah - I don't want to add it to have eachrow and eachcol as subtypes - that's not possible, I agree. But I think it's important to add it in Compat so that packages like ArraysOfArrays.jl and JuliennedArrays.jl can make their types subtypes of AbstractSlices while still supporting Julia LTS. And so we can do something like

@static if isdefined(Base, :Slices)
    const SlicedArrayLike = AbstractSlices
else
    const SlicedArrayLike = Union{EachRow,EachCol,EachSlice,AbstractSlices}
end

in ArrayInterfaceCore (prototyping this here). That will then allow code that can take advantage of slices array to dispatch on SlicedArrayLike, which will cover the legacy eachrow and eachcol, as well as the Julia v1.9 Slices and subtypes of AbstractSlices.

For that to work, we need an AbstractSlices that's available from Julia v1.6 on, via Compat.

@oschulz

oschulz commented Nov 12, 2023

Copy link
Copy Markdown

Bump - is there support for adding this to Compat?

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.

4 participants