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.

Sign up for GitHub

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

eachslice + collect #73

Closed
wants to merge 3 commits into from
Closed

eachslice + collect #73

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1c24eb
eachslice + collect
Oct 5, 2019
7914d09
Update src/functions.jl
mcabbott Oct 11, 2019
97d070d
rm broken
mcabbott Oct 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 31 additions & 14 deletions src/functions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function nameddimsarray_result(original_nda, reduced_data, reduction_dims::Colon
return reduced_data
end

###################################################################################
################################################
# Overloads

# 1 Arg
Expand Down Expand Up @@ -48,19 +48,6 @@ for (mod, funs) in (
end
end

if VERSION > v"1.1-"
function Base.eachslice(a::NamedDimsArray{L}; dims, kwargs...) where L
numerical_dims = dim(a, dims)
slices = eachslice(parent(a); dims=numerical_dims, kwargs...)
return Base.Generator(slices) do slice
# For unknown reasons (something to do with hoisting?) having this in the
# function passed to `Generator` actually results in less memory being allocated
names = remaining_dimnames_after_dropping(L, numerical_dims)
return NamedDimsArray(slice, names)
end
end
end

# 1 arg before - no default for `dims` keyword
for (mod, funs) in (
(:Base, (:mapslices,)),
Expand Down Expand Up @@ -110,3 +97,33 @@ function Base.append!(A::NamedDimsArray{L,T,1}, B::AbstractVector) where {L,T}
data = append!(parent(A), unname(B))
return NamedDimsArray{newL}(data)
end

################################################
# Generators

if VERSION > v"1.1-"
mcabbott marked this conversation as resolved.
Show resolved Hide resolved
Base.eachslice(A::NamedDimsArray; dims) = _eachslice(A, dims)
else
eachcol(A::AbstractVecOrMat) = (view(A, :, i) for i in axes(A, 2))
eachrow(A::AbstractVecOrMat) = (view(A, i, :) for i in axes(A, 1))
# every line identical to Base, but no _eachslice(A, dims) to disatch on.
Copy link
Contributor

Choose a reason for hiding this comment

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

@oxinabox previously objected to copying Base code here, so I'll defer to him on this

Copy link
Member

Choose a reason for hiding this comment

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

I was wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Other approaches are possible, I see that Compat.jl does have eachslice etc. I don't know what you think of depending on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that Compat.jl does have eachslice etc. I don't know what you think of depending on that.

Now that Compat.jl is for Julia v1.0+ I'd be happy to use it, rather than copy code here :)

eachslice(A::AbstractArray; dims) = _eachslice(A, dims)
end

function _eachslice(A::AbstractArray, dims::Symbol)
numerical_dims = dim(A, dims)
return _eachslice(A, numerical_dims)
end
function _eachslice(A::AbstractArray, dims::Tuple)
length(dims) == 1 || throw(ArgumentError("only single dimensions are supported"))
return _eachslice(A, first(dims))
end
@inline function _eachslice(A::AbstractArray, dim::Int)
dim <= ndims(A) || throw(DimensionMismatch("A doesn't have $dim dimensions"))
idx1, idx2 = ntuple(d->(:), dim-1), ntuple(d->(:), ndims(A)-dim)
mcabbott marked this conversation as resolved.
Show resolved Hide resolved
return (view(A, idx1..., i, idx2...) for i in axes(A, dim))
mcabbott marked this conversation as resolved.
Show resolved Hide resolved
end

function Base.collect(itr::Base.Generator{<:NamedDimsArray{L}}) where {L}
NamedDimsArray{L}(collect(Base.Generator(itr.f, parent(itr.iter))))
end
26 changes: 25 additions & 1 deletion test/functions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ using Statistics
end

@testset "eachslice" begin
if VERSION > v"1.1-"
if VERSION < v"1.1-"
using NamedDims: eachslice
end
slices = [[111 121; 211 221], [112 122; 212 222]]
a = cat(slices...; dims=3)
nda = NamedDimsArray(a, (:a, :b, :c))
Expand All @@ -67,7 +69,29 @@ using Statistics
names(first(eachslice(nda; dims=2))) ==
(:a, :c)
)
end

@testset "eachcol, eachrow" begin
if VERSION < v"1.1-"
using NamedDims: eachrow, eachcol
end
nda = NamedDimsArray([10 20; 31 40], (:x, :y))

@test names(first(eachcol(nda))) == (:x,)
@test names(first(eachrow(nda))) == (:y,)

@test_broken names(collect(eachcol(nda))) == (:y,)
@test_broken names(collect(eachrow(nda))) == (:y,)
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these test_broken?
If there's a good reason, let's open an issue to fix them, and comment a link to that issue here

Copy link
Member

@oxinabox oxinabox Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't seem like it is broken at all.
that is a normal vector of named vectors.
I think I would find it surprising if it wasn't.

Copy link
Collaborator Author

@mcabbott mcabbott Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think the name of the generator dimension would ideally be passed along, as is now done for [f(x) for x in nda]. But I couldn't see a way to make this work without JuliaLang/julia#32310 .

end

@testset "collect" begin
ndv = NamedDimsArray([1, 9, 7, 3], :vec)
@test names([sqrt(x) for x in ndv]) == (:vec,)

nda = NamedDimsArray([10 20; 31 40], (:x, :y))
@test names([x^2 for x in nda]) == (:x, :y)

@test_broken names([x^2 + y for x in nda, y in ndv]) == (:x, :y, :vec)
mcabbott marked this conversation as resolved.
Show resolved Hide resolved
end

@testset "mapslices" begin
Expand Down