Breaking: error on unordered or reverse indices

[rafaqz]

closes #386

[rafaqz]

@tiemvanderdeure I addedd global switch for this and made sure @inbounds would also skip the check. But this is the last really bad corner of this package (that you can break lookups without an error) and I think we should close this loophole. Want to review?

[tiemvanderdeure]

Agree that this one is pretty tricky right now and we need some better solution.

It's a bit hard to see how often this problem would come up in practice. My initial thoughts are:

• I like the two options of going around the check. @inbounds doesn't work if you just run something in the REPL (like @inbounds A2 = A[2:-1:1,:]) still errors, which would be a bit annoying.
• We should probably address this at the same time as regular/irregular lookups. Something like A[Y([1,3,4])] still produces a Regular lookup that is actually irregular and lies about it's step
• I think it would make sense to consider narrowing the cases where dimensions are automatically assigned ForwardOrdered. Maybe strings and symbols should by default be considered unordered and we check order only for subtypes of Real

[rafaqz]

Yeah those all make sense. Probably any AbstractVector{Int} that isn't AbstractRange should convert a lookup to Irregular.

And yes probably most String categories are not intended to be ordered.

Want to make issues for those separately?

[rafaqz]

We should probably address this at the same time as regular/irregular lookups. Something like A[Y([1,3,4])] still produces a Regular lookup that is actually irregular and lies about it's step

This is actually not the case, at least on breaking. Fixed already!

[tiemvanderdeure]

This is actually not the case, at least on breaking. Fixed already!

Oops you're right.

There's still some inconsistency in that when you index into a Regular lookups with a vector, it is turned into an Irregular lookup, while if you index into a Ordered lookup, it either remains Ordered or throws an error. I understand that with the issorted check it would be type unstable, but another solution would be to just always make it Unordered. Again I'm not sure how often this would come up in practice - at least with spatial data it would be pretty rare to index into a dimension with a vector, cropping is much more common.

I'll make an issue about the question when something should be ordered.

[rafaqz]

The reason for the difference is switching to Irregular just changes the approach. But switching to Unordered changes the perfornance, possibly buy orders of magnitude on a large array because we have to use find rather than searchsorted.

The reason to check is to not lose seachsorted. I'm making the assumption that changing the array size with getindex/view happens much less often than indexing a single value with At or Near. And for getindex the cost is already so high for allocating a new array that checking the sort order is irrelevant.

So checking the order is preferable to switching to unordered.

There is another approach... I considered adding a field to ForwardOrdered etc so they can be used as wrappers in indexing, like the users saying "I know the order of this Array". But its actually unclear where you need them... like UnitRange doesn't but StepRange does. so IDK.

So I kind of think that sorted ascending is the 95% use case when using an AbstractArray{Int} and that should just work without any downsides like slow lookups.

[tiemvanderdeure]

There is another approach... I considered adding a field to ForwardOrdered etc so they can be used as wrappers in indexing, like the users saying "I know the order of this Array".

I can't immediately see which use cases there would be for this. If you really know the order you can always use set-

But switching to Unordered changes the perfornance, possibly buy orders of magnitude on a large array because we have to use find rather than searchsorted.

This makes good sense! I don't know if we should consider a global switch that makes indexing behaves this way. But maybe there are just too few cases. (and we can always add functionality like that in a minor release)

Another idea is to add something like reformat, that is similar to format but also checks if dimensions are lying about their ordering and regular-ness. So if you don't care about type stability then you can drop the checks and then use this to make sure you always get correct results.

But clearly you've thought more about this than I have and I can't think of a way to much improve this more fundamentally.

[rafaqz]

The use case is to skip all of the checks.

Like A[X=ForwardOrdereded([1,2,3])]. I was also thinking of this to pass through from selectors so their outputs are not checked.

[tiemvanderdeure]

Like A[X=ForwardOrdereded([1,2,3])]

Okay I see - but 1) wouldn't you need to then still wrap that in At? and 2) doesn't @inbounds A[X=[1,2,3]] already work in that case? (even if that only works inside a function)

[rafaqz]

wouldn't you need to then still wrap that in At?

Those are Int so no? but that could work too.

doesn't @inbounds A[X=[1,2,3]] already work in that case?

Yeah, but Unordered() and ReverseOrdered() would also work? and this would always work.

Thats my point overall, this was a 20 second example