refactor rand on UnitRange / AbstractArray #9324
Conversation
|
@FeodorFitsner is it a bug that AppVeyor considers this PR (21a73b9) non-mergeable? Github and Travis think it's okay. |
|
Maybe it was kind of racing condition. AppVeyor checks virtual "merge" commit for existence using this call to GitHub API: When commit not found (404) PR is considered as non-mergeable. Maybe at the time of checking it wasn't there yet, because now it returns commit details. |
|
Guess it was just a network hiccup then. I think I've seen the same thing happen once before, maybe Travis is trying multiple times before giving up? Or it might be working differently because they're a Github service vs AppVeyor works off a webhook. Let me see what the contents of the hook delivery were. |
|
I think small delay before checking virtual commit might be a solution (or retry). I'm going to add that with the next update. |
|
@rfourquet Could you explain a little bit more about the refactoring in the commit? I think this looks good. @ivarne ? |
|
I don't see anything I would question right now. A few comments would probably make it easier to track down what is happening, and might help me see more potential issues. |
|
Thanks for looking at this! Ok I will explain more is the commit. @ivarne, do you mean adding comments in the source code? Really my motivation was simplification/removing duplication; previously I tried this by using |
rfourquet
force-pushed
the
rf/rand-ranges
branch
from
21a73b9
to
3416554
Compare
This is another approach to the (non-merged) refactoring part of #8309 and #8649. The goal is to reduce to the minimum the different code paths taken by UnitRange and AbstractArray (UnitRange ranges are handled differently so that those with overflowing length still work). In particular two rand! method are merged into one. Previously, RangeGenerator objects could create (scalar of array of) random values in a range a:b, taking care of creating first a random value in 0:b-a and then adding a. Choosing a random value in an AbstractArray A was then using A[rand(1:length(A))]. Here, RangeGenerator is changed to only handle the creation of random values in a range 0:k, and determining the right value of k (length(A)-1 or b-a) and picking the right element using the random value v (A[1+v] or a+v) is left to separate (and minimal) methods. Hence Range and AbstractArray are handled as uniformly as possible, given that we still want to support ranges like typemin(Int):typemax(Int) for which length overflows.
|
I updated the commit message, tell me if that helps or if I should try again. |
|
hmm, a bit late for a bump here 😉 |
This is another approach to the (non-merged) refactoring part of #8309 and #8649.
The goal is to reduce to the minimum the different code paths taken by
UnitRangeandAbstractArray(UnitRanges are handled differently so that those with overflowinglengthstill work).In particular two
rand!method are merged into one.Only
rand(::UnitRange{Bool})becomes slower (by 10-15%), as it is no more in the group of specially supported ranges, because arithmetic on them doesn't produceBool, so they would need individual support (which I can easily add if requested).Note: This change suggests another one (0c6b81a): merge all other array filling methods.