Deprecate sumabs, sumabs2, minabs, maxabs #19616
Conversation
| maxabs{T<:Number}(x::AbstractSparseVector{T}) = maxabs(nonzeros(x)) | ||
| minabs{T<:Number}(x::AbstractSparseVector{T}) = nnz(x) < length(x) ? abs(zero(T)) : minabs(nonzeros(x)) | ||
| maximum{T<:Number}(f, x::AbstractSparseVector{T}) = maximum(f, nonzeros(x)) | ||
| minimum{T<:Number}(f, x::AbstractSparseVector{T}) = nnz(x) < length(x) ? f(zero(T)) : minimum(f, nonzeros(x)) |
There was a problem hiding this comment.
this isn't always true for functions other than abs
There was a problem hiding this comment.
Ah yeah. Could just do ::typeof(abs) and ::typeof(abs2) then, right?
| @@ -1284,8 +1284,7 @@ end | |||
| ### Reduction | |||
|
|
|||
| sum(x::AbstractSparseVector) = sum(nonzeros(x)) | |||
| sumabs(x::AbstractSparseVector) = sumabs(nonzeros(x)) | |||
| sumabs2(x::AbstractSparseVector) = sumabs2(nonzeros(x)) | |||
| sum(f, x::AbstractSparseVector) = sum(f, nonzeros(x)) | |||
There was a problem hiding this comment.
this isn't true for functions that aren't zero-preserving
ararslan
force-pushed
the
aa/deprecate-sumabs-et-al
branch
from
4c8f064 to
937c970
Compare
| @@ -1305,8 +1303,14 @@ function minimum{T<:Real}(x::AbstractSparseVector{T}) | |||
| min(zero(T), minimum(nonzeros(x))))::T | |||
| end | |||
|
|
|||
| maxabs{T<:Number}(x::AbstractSparseVector{T}) = maxabs(nonzeros(x)) | |||
| minabs{T<:Number}(x::AbstractSparseVector{T}) = nnz(x) < length(x) ? abs(zero(T)) : minabs(nonzeros(x)) | |||
| for f in [:sum, :maximum, :minimum], op in [:abs, :abs2] | |||
There was a problem hiding this comment.
I think this is fine for now since it covers what we already have but I was wondering if we could get something more general (probably not in this PR) with something like
mapreduce(f, op, x::AbstractSparseVector) = op(mapreduce(f, op, nonzeros(x)), repeat(op, f(zero(x[1])), countnz(x)))where the repeat functions could get specialized, e.g.
repeat(::typeof(+) , x, n::Integer) = x*nThere are probably problems with this approach that I haven't seen yet but it seems nice that it can handle arbitrary return values of the zero input and be fast for the important cases.
There was a problem hiding this comment.
I'm far from an expert on the intricacies of dealing with sparse structures, so if it's alright I think I'd prefer to punt that to a different PR assuming what I've done here is fine in the interim.
There was a problem hiding this comment.
Sure. I think that is the right approach. Just wanted to share my half-baked idea.
There was a problem hiding this comment.
Could be a nice optimization, but needs op to commute, right?
There was a problem hiding this comment.
@martinholters Right. The definition strengthens the assumptions which wouldn't be okay. I'll say the thing we always say in situations like these: traits could help.
| @deprecate maxabs(x) maximum(abs, x) | ||
| @deprecate maxabs(A, region) maximum(abs, A, region) | ||
| @deprecate sumabs!(r, A) sum!(abs, r, A) | ||
| @deprecate sumabs!(r, A; init=false) sum!(abs, r, A; init=false) |
There was a problem hiding this comment.
guess the rhs here should just say init
There was a problem hiding this comment.
Ah, I knew there had to be a better way to do it than this. Thanks, I'll try that.
There was a problem hiding this comment.
This should be tested manually on some inputs where different init values would make a difference, but I think the way it should probably be is init=falseinit=true on the left, and init=init on the right
There was a problem hiding this comment.
Why init=false on the left?
There was a problem hiding this comment.
that's the default value, otherwise init is undefined
There was a problem hiding this comment.
I thought init=true was the default?
There was a problem hiding this comment.
then I didn't read the deleted lines in the diff closely enough - the deprecation should try to preserve the signature and behavior of the old method
|
Really nice pr, @ararslan :) |
|
Thanks @kshyatt! Glad I can help. |
|
Benchmarks should be updated to not call the deprecated methods, yes. |
As suggested by @andreasnoack in #19598 (comment).
This PR deprecates these functions, as well as their in-place counterparts, in favor of
sum,minimum, andmaximumwith function arguments.