[WIP] More generalindexing [3] #15679
Conversation
| @@ -1544,13 +1544,13 @@ function pmap(f, lsts...; err_retry=true, err_stop=false, pids = workers()) | |||
| nextidx = 0 | |||
| getnextidx() = (nextidx += 1) | |||
|
|
|||
| states = [start(lsts[idx]) for idx in 1:len] | |||
| states = [start(lst) for lst in lsts] | |||
There was a problem hiding this comment.
Even shorter: map(start, lsts).
There was a problem hiding this comment.
Surprisingly, changing it to map(start, lsts) leads to an error in tests (see log). I will change it to the original code, but it may uncover some other issue.
| @@ -1544,7 +1544,7 @@ function pmap(f, lsts...; err_retry=true, err_stop=false, pids = workers()) | |||
| nextidx = 0 | |||
| getnextidx() = (nextidx += 1) | |||
|
|
|||
| states = [start(lsts[idx]) for idx in 1:len] | |||
| states = map(start, lsts) | |||
There was a problem hiding this comment.
One risk with using map here is that you can't guarantee that states is an array. For example, if lsts is a tuple, then states will be a tuple, and the later line that sets states[i] = ... will fail.
There was a problem hiding this comment.
Correct, reverting it to the original code fixes the error (at least locally).
|
Mostly looks good. I noticed there's a travis error; might be related to the point about |
|
@timholy Do you have any suggestions about how to do performance testing for this PR? |
|
Performance regressions seem very unlikely here, but lets |
|
Your benchmark job has completed, but no benchmarks were actually executed. Perhaps your tag predicate contains mispelled tags? cc @jrevels |
|
|
|
Thanks, Jarrett. |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
Looks good, thanks! |
One more step towards filling up this list, this time from
markdown/*toregex.jl.This is marked as WIP since there's a number of discussable changes (see
fixme (iter)comments with question mark) for which I'd be happy to get comments. Given previous PRs, I also suggest not to merge these changes until at least 2 pairs of eyes don't check it out.Also I couldn't find any "standard" performance tests, but it would be very nice to make sure the PR doesn't introduce any performance regression. I hope folks here can run such tests or point me to a proper way to do it.
I skipped several files for particular reasons described below. Please, take another look at them if you believe we can improve them further. The files are:
Works on
cellswith unspecified type. Seems to expect array of arrays, but no guarantee of consistency. Anyway, looks like internal stuff so unlikely to change default array format.All loops over internal data structures (e.g.
Messages,Field, etc), considered unlikely to change.Direct comment not to use
eachindexthere :)for i=1:length(V)whereVis aVector, so equivalent toeachindexanyway.cc: @timholy @nalimilan @KristofferC