eachslice + collect #73

mcabbott · 2019-10-10T11:42:40Z

I believe this closes #59 and #34.

I ‘m not sure why write a custom generator for eachslice. This PR changes it to just looks up dims, then run the same code as base:

julia> ab = NamedDimsArray((1:3) .+ zeros(Int,3)', (:a, :b));

julia> @btime eachslice($ab, dims=:b); # master: 319.215 ns (5 allocations: 112 bytes)
  220.690 ns (3 allocations: 64 bytes) 

julia> @btime eachslice($(parent(ab)), dims=2);
  216.897 ns (3 allocations: 64 bytes)

julia> @btime [sum(x)^2 for x in eachslice($ab, dims=:b)]; # master: 14.636 μs (40 allocations: 1.73 KiB)
  383.851 ns (8 allocations: 336 bytes)

Still much slower than eachcol, for which there is no special code at all:

julia> NamedDims.names(first(eachcol(ab)))
(:a,)

julia> @btime eachcol($ab);
  12.992 ns (2 allocations: 48 bytes)

julia> @btime [sum(x)^2 for x in eachcol($ab)];
  76.208 ns (7 allocations: 320 bytes)

It also adds the simplest possible overload of collect so that simple comprehensions will keep names. Doesn’t work on generators of generators, nor products:

julia> [x^2 for x in ab]
3×3 NamedDimsArray{(:a, :b),Int64,2,Array{Int64,2}}:
 1  1  1
 4  4  4
 9  9  9

julia> NamedDims.names([sum(x)^2 for x in eachcol(ab)]) # doesn't work yet
(:_,)

codecov · 2019-10-10T16:09:30Z

Codecov Report

Merging #73 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   86.63%   86.53%   -0.11%     
==========================================
  Files           8        8              
  Lines         247      260      +13     
==========================================
+ Hits          214      225      +11     
- Misses         33       35       +2

Impacted Files	Coverage Δ
src/functions.jl	`97.67% <100%> (+0.37%)`	⬆️
src/name_core.jl	`90.24% <0%> (-1.76%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26e974b...97d070d. Read the comment docs.

nickrobinson251

thanks!

src/functions.jl

nickrobinson251 · 2019-10-11T09:10:06Z

src/functions.jl

+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.


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

I was wrong

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

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 :)

nickrobinson251 · 2019-10-11T09:10:56Z

test/functions.jl

+        @test names(first(eachrow(nda))) == (:y,)
+
+        @test_broken names(collect(eachcol(nda))) == (:y,)
+        @test_broken names(collect(eachrow(nda))) == (:y,)


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

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.

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 .

test/functions.jl

oxinabox · 2019-10-11T09:16:09Z

It also adds the simplest possible overload of collect so that simple comprehensions will keep names. Doesn’t work on generators of generators, nor products:

Can this be in a seperare PR, it is a more contentious feature that IDK how I feel about right now

src/functions.jl

Co-Authored-By: Lyndon White <[email protected]>

mcabbott · 2019-10-11T10:09:05Z

Perhaps we should have an issue for how many more general generators can or should be handled? Making [sqrt(x) for x in ab] behave like map(sqrt, ab) seemed straightforward.
Edit: perhaps more general maps, like map(+, ab, rand(3,3)), which also currently forgets names, fall in the same basket.

mcabbott · 2019-10-14T08:02:16Z

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

OK then perhaps I close this, and leave you to make a Compat PR.

it is a more contentious feature ...

If you believe map & collected generators ought not to simply propagate names along, then perhaps open a discussion issue?

eachslice + collect

e1c24eb

nickrobinson251 reviewed Oct 11, 2019

View reviewed changes

nickrobinson251 requested a review from oxinabox October 11, 2019 09:11

oxinabox reviewed Oct 11, 2019

View reviewed changes

src/functions.jl Show resolved Hide resolved

oxinabox reviewed Oct 11, 2019

View reviewed changes

src/functions.jl Show resolved Hide resolved

mcabbott and others added 2 commits October 11, 2019 11:37

Update src/functions.jl

7914d09

Co-Authored-By: Lyndon White <[email protected]>

rm broken

97d070d

mcabbott closed this Oct 14, 2019

oxinabox mentioned this pull request Oct 14, 2019

What should the behavour of map and collect be? #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eachslice + collect #73

eachslice + collect #73

mcabbott commented Oct 10, 2019

codecov bot commented Oct 10, 2019 •

edited

Loading

nickrobinson251 left a comment

nickrobinson251 Oct 11, 2019

oxinabox Oct 11, 2019

mcabbott Oct 11, 2019

nickrobinson251 Oct 13, 2019

nickrobinson251 Oct 11, 2019

oxinabox Oct 11, 2019 •

edited

Loading

mcabbott Oct 11, 2019 •

edited

Loading

oxinabox commented Oct 11, 2019

mcabbott commented Oct 11, 2019 •

edited

Loading

mcabbott commented Oct 14, 2019

eachslice + collect #73

eachslice + collect #73

Conversation

mcabbott commented Oct 10, 2019

codecov bot commented Oct 10, 2019 • edited Loading

Codecov Report

nickrobinson251 left a comment

Choose a reason for hiding this comment

nickrobinson251 Oct 11, 2019

Choose a reason for hiding this comment

oxinabox Oct 11, 2019

Choose a reason for hiding this comment

mcabbott Oct 11, 2019

Choose a reason for hiding this comment

nickrobinson251 Oct 13, 2019

Choose a reason for hiding this comment

nickrobinson251 Oct 11, 2019

Choose a reason for hiding this comment

oxinabox Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

mcabbott Oct 11, 2019 • edited Loading

Choose a reason for hiding this comment

oxinabox commented Oct 11, 2019

mcabbott commented Oct 11, 2019 • edited Loading

mcabbott commented Oct 14, 2019

codecov bot commented Oct 10, 2019 •

edited

Loading

oxinabox Oct 11, 2019 •

edited

Loading

mcabbott Oct 11, 2019 •

edited

Loading

mcabbott commented Oct 11, 2019 •

edited

Loading