Rigorous index handling: boundschecks and indexstyle

[lkdvos]

This PR implements indexing in a style more similar to how Base handles this, with canonical IndexStyle options and a more fixed callchain of converting different index types into eachother.
Additionally, this (re)introduces boundschecking on sparse arrays.

I've updated the version as if this is a breaking change, but the breaking changes are quite minor: the output type of some of the iterators has changed a bit.
In principle I think we might be able to say this is not breaking, but I wanted to be slightly cautious about this.

[codecov]

Codecov Report

Attention: Patch coverage is 80.74534% with 31 lines in your changes missing coverage. Please review.

Project coverage is 75.65%. Comparing base (ebb3f81) to head (5cb5c01).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/indexing.jl	82.22%	24 Missing ⚠️
src/wrappers.jl	60.00%	4 Missing ⚠️
src/abstractsparsearrayinterface.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   75.42%   75.65%   +0.23%     
==========================================
  Files           7        8       +1     
  Lines         476      571      +95     
==========================================
+ Hits          359      432      +73     
- Misses        117      139      +22     

Flag	Coverage Δ
docs	34.27% <32.91%> (-0.97%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lkdvos]

I'm not entirely sure how to fix the documentation and literate workflows concerning the compat entries.

[mtfishman]

I'm not entirely sure how to fix the documentation and literate workflows concerning the compat entries.

I think you just need to bump the SparseArraysBase.jl version in docs/Project.toml.

[mtfishman]

I've updated the version as if this is a breaking change, but the breaking changes are quite minor: the output type of some of the iterators has changed a bit. In principle I think we might be able to say this is not breaking, but I wanted to be slightly cautious about this.

What if you try not marking it as a breaking change and see if the downstream tests pass? If they pass, I'd feel comfortable not marking it as breaking. Ideally packages wouldn't rely on the types of the iterators, just the values they iterate over.

[lkdvos]

I think this is now completely backwards compatible, although there are some small quirks to make this work.

I'd say that the main thing which isn't super elegant is the interplay between having
a) people now only have to define a cartesianindex or linearindex method, depending on IndexStyle
b) implementing fallback definitions for AbstractArray, where you don't know the indexstyle and therefore don't know the correct signature to implement.

I somewhat worked around this, which on the surface looks like code duplication but I think most of it is required to make the dispatching work correctly.
There definitely might be better ways, I'm open to suggestions.

Otherwise this should be ready for review.

[mtfishman]

I think it looks reasonable, maybe there is a way to simplify it but I can't see it right now...

If we allowed for some breaking changes, what would you be able to simplify?

[mtfishman]

b) implementing fallback definitions for AbstractArray, where you don't know the indexstyle and therefore don't know the correct signature to implement.

Could you explain this point a bit more? Can't you always check for the IndexStyle?

[lkdvos]

Could you explain this point a bit more? Can't you always check for the IndexStyle?

The situation I'm referring to is this:

Imagine I want to define a fallback isstored for a class of arrays with supertype MyArray.
If these have IndexCartesian, I would define isstored(x::MyArray{<:Any,N}, I::Vararg{Int,N}) where {N} and the rest would be handled.
If these have IndexLinear, I would define isstored(x::MyArray, i::Int) and the rest would be handled.

If I want to define a fallback for AbstractArray, I need to know which signature to implement depending on the IndexStyle, which I don't know because it might be either.
Therefore, I need to define fallbacks for both signatures, check if it is the canonical style with a missing implementation, or that it isn't the canonical style and map it to the canonical style. This feels a bit clunky but I can't really think of a better way.

[lkdvos]

If we allowed for some breaking changes, what would you be able to simplify?

I think mostly the functions that take style as the first argument, for example in the wrapper implementations there is now both eachstoredindex(A) and eachstoredindex(style, A), because the downstream packages don't necessarily support the second implementation.

I'd have to think a bit more if there are more simplifications.

[mtfishman]

Got it, thanks for the in-depth explanations. I think all of that makes sense. I think it is a bit tough to make these decisions in a vacuum, the current design seems good up to constraints on not being breaking and moves us in a good direction by supporting linear or cartesian indexing and bounds checking, and maybe as we use it more in other packages we can circle back on some of the designs.

Would some of this become simpler if we changed the interface for overloading isstored and related functions to isstored(::IndexStyle, a::AbstractArray, inds...)? I know we can't really do that with getindex/setindex! since Base doesn't do that in those cases.

[lkdvos]

Would some of this become simpler if we changed the interface for overloading isstored and related functions to isstored(::IndexStyle, a::AbstractArray, inds...)? I know we can't really do that with getindex/setindex! since Base doesn't do that in those cases.

This was indeed what I was considering, but it's hard to gauge if everything works out without just trying it out somehow, so I am just not sure.

I'm definitely okay with keeping this as-is, and re-evaluating as we go along.