More general indexes [2] #15564
More general indexes [2] #15564
Conversation
| @@ -550,7 +550,7 @@ function quantile!(q::AbstractArray, v::AbstractVector, p::AbstractArray; | |||
| end | |||
| isnan(v[end]) && throw(ArgumentError("quantiles are undefined in presence of NaNs")) | |||
|
|
|||
| for i = 1:length(p) | |||
| for i in eachindex(p) | |||
| @inbounds q[i] = _quantile(v,p[i]) | |||
There was a problem hiding this comment.
You should use two different indices for p and q, with zip(eachindex(p), eachindex(q)).
There was a problem hiding this comment.
Good idea, I've updated the code.
dfdx
force-pushed
the
more-general-indexing-2
branch
from
c38521d to
e932103
Compare
|
Looks great, many thanks! |
|
Just checking, am I correct in the number of items I checked off in #15434? |
| @@ -426,7 +426,7 @@ function selectperm!{I<:Integer}(ix::AbstractVector{I}, v::AbstractVector, | |||
| order::Ordering=Forward, | |||
| initialized::Bool=false) | |||
| if !initialized | |||
| @inbounds for i = 1:length(ix) | |||
| @inbounds for i in eachindex(ix) | |||
There was a problem hiding this comment.
I don't think this is right, you're storing the index in the permutation vector
There was a problem hiding this comment.
Didn't know about that. Do you have specific type of permutation in mind?
I'll create a PR to fix this.
There was a problem hiding this comment.
AbstractVector{I<:Integer} is the signature. similar below insortperm!
There was a problem hiding this comment.
@timholy Maybe we should be a bit more careful before merging this kind of change? As we have seen, it can be more tricky than it sounds to check that all modifications make sense.
There was a problem hiding this comment.
I am slightly uneasy about all this from a performance point of view. From what I have seen before, it is quite easy to accidentally introduce a 20-30% regression by causing something to not inline anymore. I know you have done some benchmarks with zip etc but are they valid in all cases that are being changed in all these PRs? To me, something like this would be better in a separate branch and then running the benchmark suite on the whole thing,
There was a problem hiding this comment.
Shoot, clearly I need to expand the context when reviewing these (and perhaps not do it late at night). Yes, this falls under the "relying on the representation" clause.
Slightly radical thought, but I wonder if we should consider making all array indexes return their "context", so they can be converted into some kind of canonical form? For example, as an immutable with size information, so that linear<->cartesian transformations are possible when holding just the index. I don't think we want to go there yet, but it's something to keep in mind.
@KristofferC, I take your point. I don't want to make folks tackle too much, so in the future let's just run the benchmarks even on smaller PRs.
Also keep in mind that in many cases, this could become a performance win; in general, iteration with eachindex is no slower, and sometimes much much faster, than for i = 1:n iteration.
Fix for indexing issue introduced in #15564
Here's a second PR to cover this list of subtasks (from
show.jland to the end of the list).General idea is to move from a linear integer-based indexing (i.e.
for i=1:length(A)) to more general approaches (e.g.for a in Aorfor a in eachindex(A)) to support wider range of arrays and possibly better optimize code for them.