RFC: Deprecate partial linear indexing #20079

mbauman · 2017-01-16T22:12:03Z

This is the tiniest of scalpels compared to the chainsaw that was #20040. This only deprecates the most offensive behavior, that is, the "linearization" of dimensions beyond the first. To be more specific, this means that it's still ok to index a three dimensional array with two indices. That second index, though, now cannot exceed size(A, 2). For indices between size(A, 2) and trailingsize(A, 2), this will now throw a warning.

After this deprecation goes through, we'll be able to change the behavior of end and : such that they no longer call trailingsize. But for now, indexing rand(2,3,4)[1, end] and rand(2,3,4)[1, :] will throw a warning.

This is rather inline with allowing trailing singleton dimensions. When you index a three-dimensional array with only two indices, you're implicitly treating it as a matrix, with an implicit 1 in the third dimension. This is not terribly unlike indexing a one-dimensional array with a trailing singleton dimension to treat it as a matrix.

I don't have much more time to spend on this, but the deprecation fixes required here are much more contained. In fact, I believe they're entirely limited to test/subarray.jl, test/arrayops.jl and test/abstractarray.jl.

tkelman · 2017-01-16T23:05:27Z

base/abstractarray.jl

@@ -325,15 +325,15 @@ end
 checkbounds(A::AbstractArray) = checkbounds(A, 1) # 0-d case

 """
-    checkbounds_indices(Bool, IA, I)
+    checkbounds_indices(Bool, A, IA, I)


should we deprecate the old signature? DistributedArrays, ImageCore, and ImageFiltering look like they're calling this

I'm not so sure it's easily deprecatable, especially if they both extend and call it. I was hoping I'd be safe mucking with the only unexported function in this chain.

Can't win em all, if it's ambiguous and not possible to deprecate then maybe those 3 packages get what's coming to them and deal with the breakage. Is using this function incorrect for them to be doing?

Eh, I think I'd have a hard time scolding @timholy for using a function he wrote. We can see what he says. I can look to see how DistributedArrays uses it.

Ah, yup, DArray extends it as an optimization. It'll be easy to add the new methods right alongside it.

(... and it's actually code that I originally wrote. I knew the prose in the comment looked awfully familiar. Lol.)

I don't mind changing code to match. Is the reason for wanting the array as one of the arguments to check for a linear index being the first argument? What about fixing this in the callers of checkbounds_indices instead?

I'm more concerned about the "principle" than anything else: once you know the indices of the array and the indices passed by the user, it seems you know everything you need to know. Encouraging additional dispatch on the array type just seems to beg ambiguities. OTOH, I could imagine that checking the in caller might not be as nice, so I'd trust you if you think this is the best way to go.

tkelman · 2017-01-16T23:07:44Z

base/multidimensional.jl

 end

-function checkindex{N}(::Type{Bool}, inds::Tuple, I::AbstractArray{CartesianIndex{N}})
+function checkindex{N}(::Type{Bool}, A::AbstractArray, inds::Tuple, I::AbstractArray{CartesianIndex{N}})


so only this checkindex method gets a new argument?

Yes. It's already significantly different from the others as it takes a tuple of source indices. I need some way of passing extra information to see if the index is in dimension one or not.

tkelman · 2017-01-17T02:53:56Z

src/cgutils.cpp

+                BasicBlock *partidxwarn = BasicBlock::Create(jl_LLVMContext, "partlinidxwarn");
+                Value *d = emit_arraysize_for_unsafe_dim(ainfo, ex, nidxs, nd, ctx);
+                builder.CreateCondBr(builder.CreateICmpULT(ii, d), endBB, partidx);
+


timholy

I like the scalpel. Nice job on the codegen side, BTW.

timholy · 2017-01-17T11:31:14Z

base/abstractarray.jl

@@ -325,15 +325,15 @@ end
 checkbounds(A::AbstractArray) = checkbounds(A, 1) # 0-d case

 """
-    checkbounds_indices(Bool, IA, I)
+    checkbounds_indices(Bool, A, IA, I)


I don't mind changing code to match. Is the reason for wanting the array as one of the arguments to check for a linear index being the first argument? What about fixing this in the callers of checkbounds_indices instead?

I'm more concerned about the "principle" than anything else: once you know the indices of the array and the indices passed by the user, it seems you know everything you need to know. Encouraging additional dispatch on the array type just seems to beg ambiguities. OTOH, I could imagine that checking the in caller might not be as nice, so I'd trust you if you think this is the best way to go.

mbauman · 2017-01-17T15:06:18Z

There is another way to go about this change. We could deprecate linearization in all cases where there's more than one index provided. Right now, this PR still allows linearization of the first dimension when other 0-index CartesianIndex()es are provided:

julia> const newaxis = [CartesianIndex()]
1-element Array{CartesianIndex{0},1}:
 CartesianIndex{0}(())

julia> A = rand(4,3,2)

julia> vec(A[newaxis, :]) == vec(A[:, newaxis]) == A[:]
true

This behavior is a little oblique and has proven hard to support. I don't think it's unreasonable to only permit linearization when there's only one index provided. In that case, we can make this change very simply in checkbounds(::Type{Bool}, A, ::NonCartesianIndex), and skip passing A through check_indices. The downside here is that we lose track of how many dimensions there were originally when we go to print the deprecation message.

timholy · 2017-01-17T16:16:13Z

Overall I like that plan, as long as you don't think the absence of 0-index indices will cause too many problems. (I'm not a big user of them myself, I think...)

As for the tradeoff of passing/not passing the array to checkbounds_indices vs a useful deprecation warning, I'll support whatever you choose (there are benefits either way).

mbauman · 2017-01-18T02:24:32Z

Why not both? We're digging through the stack traces in any case for depwarn… might as well make good use of it! I think I got many of the depwarns out of test… let's how this goes...

tkelman · 2017-01-18T02:29:38Z

base/deprecated.jl

-                @goto found
-            end
+            found && @goto found
+            found = lkup.func in funcsyms


these two lines look like they should be reversed to me

Nope, that's very intentional. The caller is the next stack trace after we find the symbol we're looking for.

tkelman · 2017-01-18T03:13:23Z

src/cgutils.cpp

+                Value *d = emit_arraysize_for_unsafe_dim(ainfo, ex, nidxs, nd, ctx);
+                builder.CreateCondBr(builder.CreateICmpULT(ii, d), endBB, partidx);
+
+                // We failed the normal bounds check; check to see if we're 


whitespace failure

mbauman · 2017-01-19T00:37:54Z

test/abstractarray.jl

-    @test B[:,:] == A[:,:] == reshape(1:N, shape[1], prod(shape[2:end]))
-    @test B[1:end,1:end] == A[1:end,1:end] == reshape(1:N, shape[1], prod(shape[2:end]))
+    # @test B[:,:] == A[:,:] == reshape(1:N, shape[1], prod(shape[2:end]))
+    # @test B[1:end,1:end] == A[1:end,1:end] == reshape(1:N, shape[1], prod(shape[2:end]))


Any advice on the best way to deal with these tests? They're good tests that are true and will remain to be true even after the deprecation ends and the semantics change.

We have a @test_warn functionality now, so we could have them enabled, testing for the deprecation, with a comment that say to turn them back to normal tests when the deprecation gets removed?

I initially tried that, but it doesn't quite work. It breaks when --depwarn=error. And the depwarn state is different depending upon how the tests are run (include("test/…") vs make -C test … vs make test). I suppose I can just tack on a comment with an easy-to-grep-for string on all these lines.

mbauman · 2017-01-23T00:21:20Z

@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2017-01-23T04:27:06Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

mbauman · 2017-01-23T04:49:36Z

@nanosoldier runbenchmarks(ALL, vs=":master")

mbauman · 2017-01-23T06:07:23Z

@nanosoldier runbenchmarks(ALL, vs=":master")?

nanosoldier · 2017-01-23T10:11:52Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

mbauman · 2017-01-23T19:01:15Z

I cannot locally reproduce any of the remaining regressions that @nanosoldier has flagged.

StefanKarpinski · 2017-01-24T15:42:01Z

So is the idea here that this deprecation will catch places where people are relying on this feature and then in 1.0 we can remove the feature entirely?

mbauman · 2017-01-24T16:32:40Z

Yes, that's correct (where "this feature" means the linearization of other dimensions beyond the first). It is the smallest, least disruptive step towards addressing #14770.

In the next release after 0.6, we can change bounds checking for dimension d > 1 to only allow indexing within indices(A, d). At the same time we can change the lowering of end and the index conversion of : to no longer extend beyond that dimension. And we can consider the much bigger task of only allowing 1-or-N indices.

tkelman · 2017-01-25T08:42:36Z

base/deprecated.jl

+    ln = Int(unsafe_load(cglobal(:jl_lineno, Cint)))
+    fn = unsafe_string(unsafe_load(cglobal(:jl_filename, Ptr{Cchar})))
+    if opts.depwarn == 1 # raise a warning
+        warn(msg, once=(caller != StackTraces.UNKNOWN), key=(caller,fn,ln), bt=bt,


what's the reason for changing key from caller to this tuple?

Ah, I forgot about this. It came up when I was trying to explicitly catch the depwarns. Without this, the warning misses multiple calls at different points in the same caller. Like in tests. Or large functions.

This should help make finding depreciations a little easier to do in one shot.

tkelman · 2017-01-25T12:38:46Z

merge then?

StefanKarpinski · 2017-01-25T16:41:47Z

@mbauman, please go ahead and merge at will.

* Allow multiple lookup sites in depwarn * Deprecate partial linear indexing * Add NEWS.md * Add comments on the disabled PLI tests * Ensure linearindices completely inlines (through _length)

(cherry picked from commit fbb047c) originally from #20079

mbauman mentioned this pull request Jan 16, 2017

WIP/RFH: Deprecate generalized linear indexing #20040

Closed

48 tasks

tkelman reviewed Jan 16, 2017

View reviewed changes

kshyatt added the deprecation This change introduces or involves a deprecation label Jan 16, 2017

tkelman reviewed Jan 17, 2017

View reviewed changes

timholy reviewed Jan 17, 2017

View reviewed changes

mbauman force-pushed the mb/deprecate-partial-linear-indexing branch from 58c22b7 to 3f1c7f7 Compare January 18, 2017 02:23

tkelman reviewed Jan 18, 2017

View reviewed changes

mbauman force-pushed the mb/deprecate-partial-linear-indexing branch 2 times, most recently from a12179f to f09d299 Compare January 18, 2017 16:47

mbauman changed the title ~~RFC/RFH: Deprecate partial linear indexing~~ RFC: Deprecate partial linear indexing Jan 18, 2017

mbauman mentioned this pull request Jan 18, 2017

deprecate (then remove) generalized linear indexing #14770

Closed

mbauman commented Jan 19, 2017

View reviewed changes

timholy mentioned this pull request Jan 22, 2017

Trailing-1s indexing with 0-dimensional AbstractArrays #20175

Closed

mbauman added 4 commits January 22, 2017 18:18

Allow multiple lookup sites in depwarn

f99ace8

Deprecate partial linear indexing

abd35fe

Add NEWS.md [ci skip]

5ff6bc4

Add comments on the disabled PLI tests

606c88f

mbauman force-pushed the mb/deprecate-partial-linear-indexing branch from b315664 to 606c88f Compare January 23, 2017 00:19

Ensure linearindices completely inlines (through _length)

fbb047c

mbauman added this to the 0.6.0 milestone Jan 24, 2017

tkelman reviewed Jan 25, 2017

View reviewed changes

mbauman merged commit 876549f into master Jan 25, 2017

mbauman deleted the mb/deprecate-partial-linear-indexing branch January 25, 2017 22:42

Sacha0 mentioned this pull request Jan 26, 2017

fix failing subarray test on master (partial linear indexing depwarn) #20242

Merged

StefanKarpinski mentioned this pull request Jan 26, 2017

things we should deprecate, 0.6 edition #19598

Closed

22 tasks

mbauman mentioned this pull request Jan 26, 2017

Arraypocalypse Now and Then #13157

Closed

27 tasks

This was referenced Feb 12, 2017

Separate dispatch for dropping trailing 1s (fixes for non-1 based arrays) #20573

Merged

RFC/WIP: fully deprecate partial indexing. Fixes #14770. #20600

Closed

tkelman mentioned this pull request May 2, 2017

More fixes for non-1 arrays #21251

Merged

tkelman pushed a commit that referenced this pull request May 4, 2017

Ensure linearindices completely inlines (through _length)

e1c9a4b

(cherry picked from commit fbb047c) originally from #20079

mbauman mentioned this pull request Oct 23, 2018

Support repeat at any dimension #29626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Deprecate partial linear indexing #20079

RFC: Deprecate partial linear indexing #20079

mbauman commented Jan 16, 2017 •

edited

Loading

tkelman Jan 16, 2017

mbauman Jan 16, 2017

tkelman Jan 16, 2017

mbauman Jan 16, 2017

mbauman Jan 16, 2017

mbauman Jan 16, 2017

timholy Jan 17, 2017

tkelman Jan 16, 2017

mbauman Jan 16, 2017

tkelman Jan 17, 2017

timholy left a comment

timholy Jan 17, 2017

mbauman commented Jan 17, 2017

timholy commented Jan 17, 2017

mbauman commented Jan 18, 2017

tkelman Jan 18, 2017

mbauman Jan 18, 2017

tkelman Jan 18, 2017

mbauman Jan 19, 2017

tkelman Jan 19, 2017

mbauman Jan 19, 2017

mbauman commented Jan 23, 2017

nanosoldier commented Jan 23, 2017

mbauman commented Jan 23, 2017

mbauman commented Jan 23, 2017

nanosoldier commented Jan 23, 2017

mbauman commented Jan 23, 2017

StefanKarpinski commented Jan 24, 2017

mbauman commented Jan 24, 2017

tkelman Jan 25, 2017

mbauman Jan 25, 2017

tkelman commented Jan 25, 2017

StefanKarpinski commented Jan 25, 2017

RFC: Deprecate partial linear indexing #20079

RFC: Deprecate partial linear indexing #20079

Conversation

mbauman commented Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman commented Jan 17, 2017

timholy commented Jan 17, 2017

mbauman commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman commented Jan 23, 2017

nanosoldier commented Jan 23, 2017

mbauman commented Jan 23, 2017

mbauman commented Jan 23, 2017

nanosoldier commented Jan 23, 2017

mbauman commented Jan 23, 2017

StefanKarpinski commented Jan 24, 2017

mbauman commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jan 25, 2017

StefanKarpinski commented Jan 25, 2017

mbauman commented Jan 16, 2017 •

edited

Loading