Implement _cat instead of cat

[lkdvos]

This is a slightly hacky way to bypass causing a lot of method invalidations. In particular, see the following issues:

ITensor/SparseArraysBase.jl#25
ITensor/BlockSparseArrays.jl#34

originally noticed during: ITensor/ITensors.jl#1579

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 38.41%. Comparing base (d760a1f) to head (d1cf521).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/abstractarrayinterface.jl	0.00%	1 Missing ⚠️
src/traits.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #13   +/-   ##
=======================================
  Coverage   38.41%   38.41%           
=======================================
  Files           9        9           
  Lines         315      315           
=======================================
  Hits          121      121           
  Misses        194      194           

Flag	Coverage Δ
docs	38.21% <0.00%> (ø)

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.

[mtfishman]

Great, thanks! Glad that in the end there is a relatively simple (though hacky) fix.

[mtfishman]

@lkdvos I think this should have been marked as a breaking change, since downstream libraries like BlockSparseArrays.jl are expecting @interface ::AbstractArrayInterface Base.cat(::AbstractArray...; dims) to be defined (that package doesn't use AbstractArrayOps and instead derives functions one by one).

Alternatively, thinking about this PR more, probably what we want to do is still overload:

@interface interface::AbstractArrayInterface function Base.cat(as::AbstractArray...; dims)
  # ...
end

but then downstream libraries can "derive" it by manually forwarding Base._cat(dims, as::MyArray...) = @interface interface(as...) cat(as...; dims). In that case, it should be removed from AbstractArrayOps since then it wouldn't fit the @derive code pattern. In my opinion, just because Base requires an annoying overload interface, it doesn't mean our interface functions have to.

[lkdvos]

Sorry my mistake, I somehow understood your comment as a go ahead and wanted to strike this off my to do list.

I understand what you’re saying, but I don’t think sending things back through a keyword syntax is really a good idea. The problem is that we’re introducing quite a few layers of non-trivial indirection, and this keyword actually determines the output type, so if somewhere along the way the const propagation fails this method becomes type unstable as a whole.

I would maybe say that your initial implementation and suggestion in terms of a more broadcast-like way of thinking could be better suited, although that maybe doesn’t fit in so nicely with the whole interface idea, so maybe that should then just be a separate thing?

[mtfishman]

I would maybe say that your initial implementation and suggestion in terms of a more broadcast-like way of thinking could be better suited, although that maybe doesn’t fit in so nicely with the whole interface idea, so maybe that should then just be a separate thing?

Why doesn't that fit into the interface approach? Part of the point of this package, in my perspective, is to "hijack" things we think Base is doing poorly and do them better (if Base was doing everything well with good customization points, like it does with broadcasting, this package wouldn't be needed).

[mtfishman]

I understand what you’re saying, but I don’t think sending things back through a keyword syntax is really a good idea. The problem is that we’re introducing quite a few layers of non-trivial indirection, and this keyword actually determines the output type, so if somewhere along the way the const propagation fails this method becomes type unstable as a whole.

I am slightly sympathetic to this issue, but also I think we shouldn't bend over backwards and make our interface worse because Base is doing something bad. But also I don't like being "prematurely pessimistic" about what the compiler will do, especially if it means making the code design worse.

[mtfishman]

A compromise I could see is maybe:

@interface interface::AbstractArrayInterface function Base.cat(as::AbstractArray...; dims)
  return @interface interface cat_dims(dims, as...)
end

@interface interface::AbstractArrayInterface function cat_along(dims, as::AbstractArray...)
  a_dest = similar(Cat(as...; dims))
  @interface interface cat!(a_dest, as...; dims)
  return a_dest
end

and then types that want to derive our interface version of cat can do: Base._cat(dims, as::MyArray...) = @interface interface(as...) cat_along(dims, as...). I.e. I think we should do a better job than Base and not expose _cat as a function we expect external types to overload. If we think a good interface to overload involves putting dims as an argument, that's fine, but I think we should decide that for ourselves independent of the choice Base made for that. (EDIT: Edited to reflect a new name function name idea cat_along for a version of Base.cat where the dims are past as the first argument.)

[mtfishman]

I made #17 just as a quick fix for issues I was hitting in BlockSparseArrays.jl.