Remove the "experimental" status from non-1 indexing

[timholy]

This is a start at "taking off the training wheels" on custom indexing and allow packages like OffsetArrays to support size, length, and @inbounds.

So far this is almost exclusively documentation. The bigger part of the change will be to make Base safe for this change in policy. That means that for any function that calls size or length, we either need to generalize it or call assert_oneindex(A) (new to this PR). Overwhelmingly, the main place we'll need to add this is the linalg and sparse code.

I didn't add assert_oneindex back in the 0.5 days because I hoped we might soon have traits and could have written cholfact(A::AbstractMatrix::OneIndexing). But at this point, we just need to get this done manually.

Before doing the big task, let's bikeshed the name and discuss any other issues folks can think of.

[fredrikekre]

Funny coincidence, I asked if we could do just this the other day in slack :)

let's bikeshed the name

Perhaps we can add a isonebasedfunction? And in code @assert isonebased(A) / assert(isonebased(A))

[oscardssmith]

Why can't we just do first(x)==1 or something similar?

[timholy]

I wanted to be certain there would be no runtime penalty, so wanted the type system to make the decision. But for OneTo indices, it looks like even

all1(A::AbstractArray) = all(r->first(r)==1, indices(A))

compiles down to a no-op on 0.7. I'm still not used to how good constant-propagation has gotten!

[timholy]

I'm thinking of going with @assert is_one_indexed(A). I'll start implementing this, and we can fix using sed if people come up with a better name.

[timholy]

I'm going to assume this is considered non-breaking and can be finished after the feature freeze, since all it amounts to is putting guards in place that are currently triggered by size.

[StefanKarpinski]

Yes, I think that's reasonable. Fixing this should break any code that isn't already broken.

[timholy]

OK, here's a big PR. I manually reviewed nearly every use of size, length, and @inbounds throughout Base and the src folders of stdlibs. (I skipped a few, like Markdown and Pkg, which manipulate internal data structures and for which it's hard to imagine anyone using arrays with non-1 indexing.) Can't promise I got them all, but this should be most of them.

The PR became a bit of a grab-bag because I noticed other issues as I went and generally tried to fix them. Most are fairly straightforward. The "Rework some constructors" commit (currently da19abe) changes a few type signatures and avoids specifying types on the arguments to the inner constructors. I think this is mostly cosmetic though in principle there could be issues, so might be worth a peek (CC @andreasnoack).

[StefanKarpinski]

Just to be clear, this should be entirely non-breaking, right?

[timholy]

In principle, completely non-breaking. Though it looks as if, locally, I accidentally tested the master branch rather than this one. Will fix.

The caveats: I suppose the main risk is that I put some kind of guard in place on something that formerly worked despite my impression from reading the code. My strategy here was to search for functions that call length or size on arrays; given our current recommendation, those calls should fail for arrays that have non-1 indexing. But if I misread the code then I suppose I could introduce an error. I could of course also have a typo like is_one_indexed(mistypedvarname); in that case the test itself would trigger an error that wasn't formerly there. Hopefully our tests are good enough to catch most such cases, but the risk should be acknowledged.

Changing the parametrization of a couple constructors could also be construed as breaking, though. I do think those were the intent of those constructors. But if I'm wrong I can rework that patch.

[timholy]

OK, I decided I will split out the cleanups to the LinearAlgebra constructors into a separate PR---it's a separate (and important) enough change that it merits a different set of eyeballs. That does imply that the protection isn't yet complete here, but the combination of the 2 PRs should do it.

Since this touches so many files it is a bit of a magnet for merge conflicts, so I'd like to get this in without unnecessary delay.

[timholy]

One thing to ask about: I went with the name I suggested above, is_one_indexed. Now I'm wondering if this is misleading. To make this non-breaking for objects that are not AbstractArrays but which support getindex (e.g., Tuple, SuiteSparse.Dense, and of course any package-defined objects), we need is_one_indexed(obj) = true for general objects (because code in Base and stdlib may call is_one_indexed on them). Of course that will return true even for objects that don't support indexing. Consequently, I'm a little worried that this name gives the wrong impression, saying "yes, this object supports indexing, which starts with 1."

Would its converse---something like is_non1_indexed---be clearer? Then one would use @assert !is_non1_indexed(A, B, ...) which, as a double-negative, is a bit confusing in its own right.

Another question is whether we should export this or not.

[timholy]

FYI I'm going to be out of touch starting Friday July 13. To ensure there's time to shake out any bugs after this gets merged, and to ensure that part 2 of this project gets adequate time for review, I'll merge this by tomorrow morning barring any objections. The name issue (see the post above this one) is probably the main thing to check.

[timholy]

OK, I went with @assert !Base.is_non1_indexed(A, ...) and didn't export the function. Tests pass locally, will merge as soon as this gets through CI.

[timholy]

Dang, circleci is taking a long time. I'm tempted to merge without it, given my time constraints (#24899 (comment)). Complain now or I'll hit merge shortly.

[StefanKarpinski]

As long as you get ~1 pass per platform it's fine, so Travis OR Circle CI for Linux are good enough. I've restarted the Travis 64-bit Linux run which seemed to have failed due to the ongoing httpbin flakiness.

[timholy]

OK. That test did get through printing "SUCCESS" before starting on the Pkg tests. That was were the failure seemed to occur, perhaps by just overfilling its log? I can't remember how to dig out the previous run, but I thought it looked exactly like this one: https://travis-ci.org/JuliaLang/julia/jobs/402239980. If you scroll down to the end you'll see it's almost like travis just got tired of recording the output.

[mbauman]

This still has the old name here. I find the !is_non1_indexed suboptimal. What about is_offset_indexed or has_offset_axes?

[timholy]

Oooh, I like those! Will change to has_offset_axes.

[mbauman]

I think I'd be okay with calling this has_offset_axes and then requiring axes to be defined. It's still not defined for Tuples, but I say we rectify that. Everything else that supports broadcasting needs to support axes, so it doesn't seem to be that big of a requirement.

[timholy]

It's not just Tuples, it's also SuiteSparse.Sparse and a few other similar "opaque" objects. I guess we could define axes for them too, since even <:Factorization has axes defined.

But this raises the thought that going with is_offset_indexed might be better, depending on whether we think indexing or axes is the more "fundamental" operation.

[mbauman]

Sorry I was slow on the review here! Somehow you managed to work on this during both of my recent vacations. This looks great — and will make working with Offset Arrays so much nicer.

[mbauman]

arrays

[mbauman]

Just a few doc comments, otherwise this is ready to go!

[mbauman]

has_offset_axes

[mbauman]

has_offset_axes here and immediately below, and it no longer defaults to false

[timholy]

I don't know which factors contributed most, but there's good news about the performance of OffsetArrays: https://discourse.julialang.org/t/performance-of-offsetarrays/5769/5?u=tim.holy

In the fullness of time, Julia consistently lets you write code the way it should be written.