Remove method overloads that duplicate Base/LinearAlgebra generics - #443
Remove method overloads that duplicate Base/LinearAlgebra generics#443JamesWrigley wants to merge 1 commit into
Conversation
|
Some of these are for performance and dead-code elimination, so it's worth checking that |
|
I will be surprised if the downstream tests pass |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #443 +/- ##
==========================================
- Coverage 99.91% 99.91% -0.01%
==========================================
Files 9 9
Lines 1238 1235 -3
==========================================
- Hits 1237 1234 -3
Misses 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI looks good. Do you know which methods are there for performance? |
| @inline function Base.isassigned(F::AbstractFill, i::Integer...) | ||
| @boundscheck checkbounds(Bool, F, to_indices(F, i)...) || return false | ||
| return true | ||
| end |
There was a problem hiding this comment.
The default isassigned uses a slow try-catch block. Please restore these.
| fval = f(getindex_value(x)) | ||
| return all((fval,)) | ||
| end | ||
| any(x::AbstractFill) = any(identity, x) |
There was a problem hiding this comment.
any/all will iterate over all the entries. Please restore these
There was a problem hiding this comment.
I'm not sure I follow, don't these also iterate over all the entries and are the same definitions as in Base? https://github.com/JuliaLang/julia/blob/0fe418d406adbaea5d9b28a5a8410f9e2522d028/base/reducedim.jl#L991
There was a problem hiding this comment.
Why would they???? The point of fill arrays is you know all the entries:
FillArrays.jl/src/FillArrays.jl
Line 685 in 7b96cc2
There was a problem hiding this comment.
Ah I'm blind, didn't see the any(::Function, x) definitions above it 🤦
There was a problem hiding this comment.
Hmm on second thoughts aren't these ok to be deleted since both they and the Base versions will end up calling any(::Function, ::AbstractFill)?
There was a problem hiding this comment.
One beautiful thing about julia is you can easily see what is happening, using @which or the debugger. The answer: no, Base does not call any(identity, ::AbstractFill) but instead calls _any:
[1/2] any(a; dims) at /Users/solver/.julia/juliaup/julia-1.12.6+0.aarch64.apple.darwin14/Julia-1.12.app/Contents/Resources/julia/share/julia/base/reducedim.jl:988
>988 any(a::AbstractArray; dims=:) = _any(a, dims)This eventually iterates over a loop:
[1/4] _any(f, itr, #unused#) at /Users/solver/.julia/juliaup/julia-1.12.6+0.aarch64.apple.darwin14/Julia-1.12.app/Contents/Resources/julia/share/julia/base/anyall.jl:121
120 @eval function _any(f, itr::$ItrT, ::Colon)
>121 $(ItrT === Tuple ? :(@_terminates_locally_meta) : :nothing)
122 anymissing = false
123 for x in itrI appreciate your desire to remove code but please make sure you double check that that the default actually avoids iteration.
| any(x::AbstractFill) = any(identity, x) | ||
| all(x::AbstractFill) = all(identity, x) | ||
|
|
||
| count(x::AbstractOnes{Bool}) = length(x) |
|
Most of these overloads are very clearly needed to avoid looping over all entries. |
These all have generic definitions upstream that do the same thing. Using Base's unary `-` also fixes a wee bug by preserving the axes of the inputs such that `-Fill(2, (1:3,)) == Fill(-2, (1:3,))`. The old implementation used `size()` so the result would be `-Fill(2, (3,))`.
04bf622 to
23f248c
Compare
|
I also restored the
|
3 participants
These all have generic definitions upstream that do the same thing. Using Base's unary
-also fixes a wee bug by preserving the axes of the inputs such that-Fill(2, (1:3,)) == Fill(-2, (1:3,)). The old implementation usedsize()so the result would be-Fill(2, (3,)).Discovered with help from Claude 🤖