Skip to content
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: Deprecate partial linear indexing #20079

Merged
merged 5 commits into from
Jan 25, 2017

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jan 16, 2017

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.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so only this checkindex method gets a new argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kshyatt kshyatt added the deprecation This change introduces or involves a deprecation label Jan 16, 2017
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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

mbauman commented Jan 17, 2017

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
Copy link
Member

timholy commented Jan 17, 2017

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 mbauman force-pushed the mb/deprecate-partial-linear-indexing branch from 58c22b7 to 3f1c7f7 Compare January 18, 2017 02:23
@mbauman
Copy link
Member Author

mbauman commented Jan 18, 2017

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...

@goto found
end
found && @goto found
found = lkup.func in funcsyms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two lines look like they should be reversed to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace failure

@mbauman 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 mbauman changed the title RFC/RFH: Deprecate partial linear indexing RFC: Deprecate partial linear indexing Jan 18, 2017
@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]))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mbauman force-pushed the mb/deprecate-partial-linear-indexing branch from b315664 to 606c88f Compare January 23, 2017 00:19
@mbauman
Copy link
Member Author

mbauman commented Jan 23, 2017

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

@nanosoldier
Copy link
Collaborator

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

@mbauman
Copy link
Member Author

mbauman commented Jan 23, 2017

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

@mbauman
Copy link
Member Author

mbauman commented Jan 23, 2017

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

@nanosoldier
Copy link
Collaborator

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

@mbauman
Copy link
Member Author

mbauman commented Jan 23, 2017

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

@mbauman mbauman added this to the 0.6.0 milestone Jan 24, 2017
@StefanKarpinski
Copy link
Member

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
Copy link
Member Author

mbauman commented Jan 24, 2017

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.

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

tkelman commented Jan 25, 2017

merge then?

@StefanKarpinski
Copy link
Member

@mbauman, please go ahead and merge at will.

@mbauman mbauman merged commit 876549f into master Jan 25, 2017
@mbauman mbauman deleted the mb/deprecate-partial-linear-indexing branch January 25, 2017 22:42
kleinhenz pushed a commit to kleinhenz/julia that referenced this pull request Jan 26, 2017
* 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)
kleinhenz pushed a commit to kleinhenz/julia that referenced this pull request Jan 26, 2017
* 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)
tkelman pushed a commit that referenced this pull request May 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants