-
Notifications
You must be signed in to change notification settings - Fork 14
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
eachslice + collect #73
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
test/functions.jl
Outdated
@test names(first(eachrow(nda))) == (:y,) | ||
|
||
@test_broken names(collect(eachcol(nda))) == (:y,) | ||
@test_broken names(collect(eachrow(nda))) == (:y,) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
Can this be in a seperare PR, it is a more contentious feature that IDK how I feel about right now |
Co-Authored-By: Lyndon White <oxinabox@ucc.asn.au>
Perhaps we should have an issue for how many more general generators can or should be handled? Making |
OK then perhaps I close this, and leave you to make a Compat PR.
If you believe |
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 updims
, then run the same code as base:Still much slower than
eachcol
, for which there is no special code at all: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: