Deprecate vectorized clamp methods in favor of compact broadcast syntax #18609
Conversation
| @@ -38,10 +38,10 @@ import Core.Intrinsics: sqrt_llvm, box, unbox, powi_llvm | |||
| clamp(x, lo, hi) | |||
|
|
|||
| Return `x` if `lo <= x <= hi`. If `x < lo`, return `lo`. If `x > hi`, return `hi`. Arguments | |||
| are promoted to a common type. Operates elementwise over `x` if `x` is an array. | |||
There was a problem hiding this comment.
does this need a rerun of make docs?
There was a problem hiding this comment.
That it does! Good catch. Fixed. Thanks!
| @@ -313,7 +313,7 @@ seek(io, 0) | |||
| @test readdlm(io, eltype(A)) == parent(A) | |||
|
|
|||
| amin, amax = extrema(parent(A)) | |||
| @test clamp(A, (amax+amin)/2, amax) == clamp(parent(A), (amax+amin)/2, amax) | |||
| @test clamp.(A, (amax+amin)/2, amax).parent == clamp.(parent(A), (amax+amin)/2, amax) | |||
There was a problem hiding this comment.
why does this need .parent now?
There was a problem hiding this comment.
The operation on the left produces a TestHelpers.OAs.OffsetArray (from test/TestHelpers.jl) whereas that on the right produces an Array. Comparing an OffsetArray to an Array seems to fail even when their contents are identical, hence the .parent. Thoughts? Thanks!
There was a problem hiding this comment.
and without broadcast what type did it return?
There was a problem hiding this comment.
An OffsetArray ought not to be substituted for an Array. If broadcast is not preserving offsets, then that needs to be fixed.
There was a problem hiding this comment.
The existing vectorized clamp methods return an Array subtype independent of the input type (for example, clamping a SparseMatrixCSC or OffsetArray yields an Array). On the other hand broadcast better preserves the input type (for example, clamping a SparseMatrixCSC yields a SparseMatrixCSC and the analog holds for OffsetArrays). Thoughts? Thanks!
There was a problem hiding this comment.
Since clamp is not zero-preserving, I think the old behaviour for SparseMatrixCSC was preferred.
There was a problem hiding this comment.
oh right, only zero-preserving if the bounds include the origin, but sometimes preserving structure wouldn't be type stable
There was a problem hiding this comment.
Good catch! Tricky. Special case sparse and structured matrix types?
There was a problem hiding this comment.
the broadcast code will now check if this is zero preserving right? and return dense-in-sparse-format if not?
kshyatt
added
domain:maths
|
Rebased. Best! |
|
Subsumed by #19791. |
This PR deprecates all remaining vectorized
clampmethods in favor of compact broadcast syntax. Ref. #16285, #17302, #18495, #18512, #18513, #18558, #18564, #18566, #18571 #18575, #18576, #18586, #18590, #18593, #18607, and #18608. Best!(Unlike with
float,real, etc., the remaining vectorizedclampmethods never alias their input. This PR should be less controversial than #18495, #18512, and #18513 as a result.)