-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC/WIP: fully deprecate partial indexing. Fixes #14770. #20600
Conversation
if ndims(A) <= 2 || depwarns_not_error | ||
filename = tempname() | ||
open(filename, "w") do f | ||
redirect_stderr(f) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would @test_warn
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the warning only gets issued on the first call, it would make this sensitive to which tests get assigned to which workers.
Just for clarity here, I've been trying to be very careful in my vocabulary. I would describe the behavior you're deprecating here as partial indexing. I've used the term partial linear indexing (PLI) only when there is more than one index and the last index exceeds the bounds of its dimension in order to linearly access elements in the higher dimensions. I'd like to have a cohesive vision for why we allow the indexing behaviors that we do. My vision here for indexing an N dimensional array has been:
I see a symmetry between filling 1s and dropping 1s. I've ret-conned OffsetArrays into my scheme by generalizing "filling 1s" to filling the missing dimensions with their first index. It's less clear that they should drop trailing 1s... particularly if all dimensions use the same offset. I might expect a midpoint-centered array to drop trailing 0s, for example. |
I agree with your point about symmetry. However, for OffsetArrays I'm in favor of completely nixing partial indexing; the discussion in #20573 revealed the confusing depths of that concept for OffsetArrays. Given that, I figured it would make sense to be consistent for all arrays. But as long as we can nix it for OffsetArrays, and keep the linear index for OffsetArrays starting at 1, I don't have strong feelings about what we do for OneTo arrays. |
I should add, though, that I didn't fully grok your distinction between partial indexing and partial linear indexing. If people really do like the idea of partial indexing, then I wouldn't be opposed to keeping it for OffsetArrays. To me it just seems vaguely weird to specifically support a mechanism that can only access a small portion of the array. EDIT: of course, we do support |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
But what if it's not a small portion of the array? The symmetry with trailing singleton dimensions is most clear when the last dimension(s) of the array has length 1. The most common case — a one-column matrix — happens to be saved by linear indexing. But I'd argue that it's not really doing linear indexing. It's accessing purely within the bounds of its dimension. There's no linearization of higher dimensions going on here. We could choose to eliminate linear indexing and still keep this behavior separately. Reductions of a three-dimensional array along the third dimension, for example, return a matrix embedded in a higher dimensional object. Does it really make sense to disallow accessing it like a matrix? Shouldn't you be able to pass that array to an image viewer that only accesses the first two dimensions? I suppose I come at it from the opposite direction: it seems vaguely weird to specifically disallow a mechanism that seems to just "fall out" in this simplified (but still representative) bounds checking scheme where we just check to see if the index falls within the indices of its dimension. checkbounds(A, i::Integer) = checkindex(linearindices(A), i) # explicitly support 1-D linear indexing
checkbounds(A, I...) = checkbounds_indices(indices(A), I...)
checkbounds_indices(inds) = true
checkbounds_indices(inds, i, I...) = checkindex(inds[1], i) && checkbounds_indices(trailing(inds), I...)
trailing(::Tuple{}) = OneTo(1) # explicitly support trailing singleton dimensions
trailing(x::Tuple) = tail(x) If the index isn't out-of-bounds, and if we have define a sensible meaning for it, then why add error methods? They smell funny to me. It's slightly more annoying in this case because it means that it adds an interaction between It also just seems funny that In the end, though, my support for partial indexing is more about the utility of trailing singleton dimensions than the utility of partial indexing. I'd like a cohesive story. In that vein, perhaps the real symmetry here would be to enforce that omitted dimensions are of length 1. I might be able to get behind that. |
Closed in favor of a revised #20573. |
@mbauman, I was working up against a time constraint but should have been a little more verbose above. The scheme you've come up with is impressive both in its careful design and its beautiful implementation. While it's not necessarily how I would have thought about it, your arguments are compelling, and the net result is elegant and incredibly functional. Nice work, as always! |
This is an alternative to the direction that #20573 was heading in, and a more aggressive version of #20079. #20079 deprecated partial linear indexing only when an index goes beyond the range of the last provided dimension:
whereas this issues the warning even for
A[1,1]
. However, it doesn't touch trailing 1s:The WIP part of this is to decide what to do about
A[:,:]
. Do we want to deprecate that too? Currently, this does so. There may be a couple of other loose ends, too. But I think it's mostly complete.@nanosoldier
runbenchmarks(ALL, vs = ":master")