Conversation
|
👎 we have to leave something for the hackernews threads to talk about :-) |
base/abstractarray.jl
Outdated
| checkbounds(::Type{Bool}, sz::Integer, i::Real) = 1 <= i <= sz | ||
| checkbounds(::Type{Bool}, sz::Integer, ::Colon) = true | ||
| function checkbounds(::Type{Bool}, sz::Integer, r::Range) | ||
| checkbounds(::Type{Bool}, inds::UnitRange, i) = throw(ArgumentError("unable to check bounds for indices of type $(typeof(i))")) |
There was a problem hiding this comment.
not a fan of having an argument with the same name as a closely-related function
|
👍 from me for the idea. I'm impressed that it takes so little code to implement this! Have you measured the performance impact? |
Yeah, that was my biggest concern too 😄. More seriously, are you genuinely down on this? There are cases where it's nicer to write an algorithm where indexing is not based on 1. For example, if I compute a quantity as a function of displacement, it's much prettier to write my code in terms of |
Not yet, but AFAICT there shouldn't be any to the addition of Actually using an offset seems likely to have a very small performance impact, but it should be essentially negligible compared to cache misses. |
|
I would prefer spelling out the name, it's not that much saved to abbreviate it. That said, is this actually distinct from "keys"? |
|
A secret reason I made it I also thought about |
|
Having this infrastructure in Base will certainly smooth over some issues with JuMP containers. CC @joehuchette @IainNZ |
|
In https://github.com/eschnett/FlexibleArrays.jl , I used |
|
@eschnett, thanks for the enthusiasm. However, I don't think this can return sparse ind***s:
Over at ArrayIteration I use |
|
@timholy My main concern is about arrays with a lower bound different from 1. A secondary concern would be for arrays with a non-unit stride. (This should work find with an Irregular sparse arrays are not relevant for me at the moment. However, block-sparse arrays are, where the upper bound on some dimension |
|
I like the idea of being able to write |
|
Without an enormous amount of surgery it can't be |
|
Yes, of course, that would be epic surgery. Just saying that it would be a nice Fortran feature graft. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels Edit by jrevels: Ignore the pending status - like this comment says, the job is actually complete. One of the nodes isn't sending out final statuses for some reason, looking into this now. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
See here - I just realized @nanosoldier Edit: Oh, this PR isn't from a fork, so it should've been fine in the first place. Nevermind then, sorry for the noise. |
|
I suspect there's still one corner case to go, anyway. I assumed this would get them all, but I should check more thoroughly locally. |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
@nanosoldier |
|
@mbauman, this contains yet a different implementation of bounds-checking. Now that the valid range of indices is represented by a |
|
As you review it, remember JuliaLang/www_old.julialang.org#386 (might make it easier to understand if you read that first). |
I wouldn't worry about this, @timholy – your SNR is excellent, I wouldn't mind more status updates. |
| zeta, | ||
|
|
||
| # arrays | ||
| allocate_for, |
There was a problem hiding this comment.
:( Maybe this can be combined with similar? If not, at least needs a better name.
There was a problem hiding this comment.
Yes, I guess it could simply be another "branch" of similar. Will fix.
|
@timholy The new meaning of You have added a deprecation for intersect(a::Colon,b::UnitRange) = b? |
|
I do think that's a reasonable definition to have. With regards to indices(A, d, inds) = inds
indices(A, d, ::Colon) = indices(A, d)Thoughts? |
|
I guess one problem with that definition is that we have |
|
Right now, I can barely figure out how to fix |
|
I feel your pain: index gymnastics are never fun, though it's better in Julia since we've developed composable elementary operations. I find that the main trick is "look no deeper than you absolutely have to," although perhaps with distributed arrays there's no escaping looking deep. Sounds like a case where developing easy-to-think-about elementary operations might make life easier. But this particular issue is "just" an API question. One thing to note is that this would become trivial with something like #15750: we'd have indexoffset(indx) = first(indx)-1
indexoffset(::Colon) = 0Even more generally (but less simply), we could have reindex(indx, refindx) = indx
reindex(::Colon, refindx) = refindxso what used to be |
Due to a change in the behaviour of `mapslices` (JuliaLang#16260), `median(X,k)` would mutate the underlying array. Fixes JuliaLang#17153.
This is the one part of ArrayIteration.jl that I think it's reasonable to move to Base for the julia-0.5 release. The core change is the addition of the
indsindicesfunction, which should be thought of as a sister function tosizebut instead returns a tuple containing the in-bounds indexes of an array. The fallback isBut this allows one to override
indicesfor specific types:This adds the definition and uses it in
checkbounds. The bigger job will be wiring it into the rest of Base. So I thought I'd post at this stage and see what folks think.