OffsetArrays is forwarding wrapper

[chriselrod]

Fixes some VectorizationBase failures

https://github.com/JuliaArrays/ArrayInterface.jl/runs/6760119793?check_suite_focus=true

Not sure how all these regressions got by us earlier?

[chriselrod]

Note there is still yet another VectorizationBase test failure brought on by recent ArrayInterface developments:

    [5] grouped_strided_pointer(A::Tuple{LinearAlgebra.Adjoint{Float64, Matrix{Float64}}, SizedWrapper{5, 6, Float64, LinearAlgebra.Adjoint{Float64, Matrix{Float64}}}, LinearAlgebra.Adjoint{Float64, Matrix{Float64}}}, #unused#::Val{(((1, 1), (3, 1)), ((1, 2), (2, 1)), ((2, 2), (3, 2)))})
      @ LayoutPointers ~/.julia/dev/LayoutPointers/src/grouped_strided_pointers.jl:62

[codecov]

Codecov Report

Merging #302 (6103dda) into master (0e7b4e4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #302   +/-   ##
=======================================
  Coverage   90.93%   90.93%           
=======================================
  Files           9        9           
  Lines        1401     1401           
=======================================
  Hits         1274     1274           
  Misses        127      127           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e7b4e4...6103dda. Read the comment docs.

[Tokazama]

I'm not sure if we should consider OffsetArray a forwarding wrapper because it changes indices. Maybe the problem is with how StrideIndex is constructed

[chriselrod]

Is CI using the actual PR subpackages when testing dependencies?
That would explain why VectorizationBase is still failing here, while it passed on the PR that actually broke it.

[chriselrod]

I'm not sure if we should consider OffsetArray a forwarding wrapper because it changes indices. Maybe the problem is with how StrideIndex is constructed

It would be good to have default_to_forwarding_wrapper_except_when_we_explicitly_overload_otherwise.
This is how the parent_type check was often used.
For many array types, most traits are forwarded, so it's nice to get those for free, while overloading only those that make that particular type special.

[chriselrod]

Feel free to make a PR, otherwise I'll probably just upper bound ArrayInterface.

[Tokazama]

We could just define this

ArrayInterface.stride_rank(T::Type{<:OffsetArray}) = stride_rank(parent_type(T))
ArrayInterface.contiguos_axis(T::Type{<:OffsetArray}) = contiguos_axis(parent_type(T))

There's probably a way to get contiguous_axis from stride_rank so that we don't have to worry about unwrapping anything.

[chriselrod]

We could just define this

ArrayInterface.stride_rank(T::Type{<:OffsetArray}) = stride_rank(parent_type(T))
ArrayInterface.contiguos_axis(T::Type{<:OffsetArray}) = contiguos_axis(parent_type(T))

There's probably a way to get contiguous_axis from stride_rank so that we don't have to worry about unwrapping anything.

Still, something with that meaning would be useful, as otherwise people are going to have to implement the entire API / is_forwarding_wrapper is mostly useless, outside of creating dummy array types for tests.

[Tokazama]

The issue with that approach is that if something defines it's own ArrayInterface.size but not ArrayInterface.axes it will be incorrect. I agree it's not as convenient, but it seems like the only safe way to use the trait.

On the other hand, that might be a documentation issue with axes and size because right now that would be problematic whether this trait is defined or not. Maybe we just need to more explicitly document when conditions can be violated?

Sorry, I'm not trying to be difficult. Just trying to find the solution that will work best for everyone in the long run.

[chriselrod]

Closed in favor of #303

[chriselrod]

Yeah, I think it makes sense to favor correctness, but I think it's likely the is_forwarding_wrapper is going to cause regressions in most packages that tried to implement the ArrayInterface.

[Tokazama]

It's starting to look that way. We might need some sort of intermediate solution.