Add more functions implemented with [map]reduce and [map]reducedim #263
Conversation
Codecov Report
@@ Coverage Diff @@
## master #263 +/- ##
==========================================
+ Coverage 89.14% 90.29% +1.15%
==========================================
Files 36 36
Lines 2625 2658 +33
==========================================
+ Hits 2340 2400 +60
+ Misses 285 258 -27
Continue to review full report at Codecov.
|
wsshin
changed the title
|
Just browsing, but FYI for the |
|
Didn't know about the discussion. Good to know. Thanks! |
| @@ -160,25 +160,76 @@ end | |||
| ## reducedim ## | |||
| ############### | |||
|
|
|||
| @inline reducedim(op, a::StaticArray, ::Val{D}) where {D} = mapreducedim(identity, op, a, Val{D}) | |||
| @inline reducedim(op, a::StaticArray, ::Val{D}, v0) where {D} = mapreducedim(identity, op, a, Val{D}, v0) | |||
| @inline reducedim(op, a::StaticArray, ::Type{Val{D}}) where {D} = mapreducedim(identity, op, a, Val{D}) | |||
There was a problem hiding this comment.
FYI: This possibly wasn't a typo. I had been thinking about this PR for a long time :)
However, we should probably add a if VERSION < v"0.7" to support both versions of Julia, or something (or simply support both syntaxes?)
There was a problem hiding this comment.
But having to use Val(D) in reducedim and Val{D} in mapreducedim seems confusing and inconsistent. I don't see the use of Val(D) and ::Val{D} anywhere else in StaticArrays, either. Wouldn't it be better to handle this in a separate PR collectively?
There was a problem hiding this comment.
OK, so what I would like is if StaticArrays used Val{D} everywhere in v0.6 and Val(D) everywhere in v0.7. (I suppose it's OK if the v0.6 version also supports Val{D}(), partly to avoid this being a breaking change)
If you were to implement that here, that would be awesome (as it seems pertinent to the change already done on this line) - however, let me know if you'd prefer to not do that at the moment, and I'll get to it later.
There was a problem hiding this comment.
When making this change, I would like to have things like
default_similar_type{T,S,D}(::Type{T}, s::Size{S}, ::Type{Val{D}})in abstractarray.jl change to
default_similar_type{T,S,D}(::Type{T}, s::Size{S}, ::Val{D})as well. Because such a change is outside mapreduce.jl, I think creating a separate PR dedicated to ::Type{Val{D}} -> ::Val{D} would be cleaner... So yes, I would prefer not to make this change at the moment.
src/mapreduce.jl
Outdated
| # with an initial value v0 = true and false. | ||
| # | ||
| # 4. Some implementations (e.g., count(a, Val{D})) are commented out because corresponding | ||
| # Base functions (e.g., count(a, D)) do not exist yet. |
There was a problem hiding this comment.
Yet? Meaning they are mooted for v0.7?
(We are allowed to be a bit more agile and move a bit beyond Base wherever it makes sense, so long as we don't break anything that tends to work for AbstractArray)
There was a problem hiding this comment.
5172c39 uncomments these functions!
|
Basically looks good to me. Thanks @wsshin. I guess we may as well try and support the syntax for v0.6 and v1.0? (Ideally, I'd like to support multiple versions of Julia - in the past we've been hampered by internal compiler changes meaning different approaches are valid/optimal, but so far I am not really expecting this for v0.7/v1.0). |
|
Wait. While trying to uncomment |
|
OK |
|
The bug I was referring to was ignorance of the possibility of element type change in One question, though. I used |
|
Feel free to use either. I think there are historic reasons surrounding this... something about one being inferable and the other not (but these days they should both be fine). BTW, I'd be most happy with the idea of just letting the natural types fall out, rather than using inference. At the moment we can't do this everywhere since we'd need a "eltypeless" constructor for every |
|
I think I know what you mean by "letting the natural types fall out", but just in case, could you point me to a couple of examples of this practice in StaticArrays, if any? |
|
Constructors are one case, things get promoted to a common type and an output is generated. I think complete reductions also might do this? |
|
Hmmm... Not sure if I understood what you meant. Could you be more specific about how to apply your idea in |
|
I don't think you can implement this idea (at this stage) for partial reductions because the infrastructure isn't available. I was just expressing an idea of where I'd like to get to. |
|
I see. Then can we merge this now? |
|
Done! |
This PR adds more functions implemented with
[map]reduceand[map]reducedimtosrc/mapreduce.jl:iszerosum,prod,all,anyprod,count,all,any,minimum,maximumNote that
Basealready has these functions.Additionally, this PR corrects the following typos:
reducedim(op, a::StaticArray, ::Val{D})toreducedim(op, a::StaticArray, ::Type{Val{D}})reducedim(op, a::StaticArray, ::Val{D}, v0)toreducedim(op, a::StaticArray, ::Type{Val{D}}, v0)