search

JuliaArrays / OffsetArrays.jl

Fortran-like arrays with arbitrary, zero or negative starting indices.
Other
198 stars 40 forks source link

Depend on ArrayInterface #268

Open Tokazama opened 3 years ago

Tokazama commented 3 years ago

This is an initial attempt to start getting rid of Requires for ArrayInterface.

codecov[bot] commented 3 years ago

Codecov Report

Merging #268 (b63cc5e) into master (05cae5c) will decrease coverage by 2.92%. The diff coverage is 0.00%.

:exclamation: Current head b63cc5e differs from pull request most recent head fc2fda4. Consider uploading reports for the commit fc2fda4 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   96.51%   93.58%   -2.93%     
==========================================
  Files           5        5              
  Lines         459      468       +9     
==========================================
- Hits          443      438       -5     
- Misses         16       30      +14
Impacted Files Coverage Δ
src/OffsetArrays.jl 94.39% <0.00%> (-4.01%) :arrow_down:
src/axes.jl 98.73% <0.00%> (-1.27%) :arrow_down:

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 05cae5c...fc2fda4. Read the comment docs.

johnnychen94 commented 3 years ago

I'm not sure if this is the right place to make changes, just throw some concerns here:

Edit:

Introducing non-standard library dependency would make updating codes there very difficult.

If we can keep the codes in a separate file (say, src/arrayinterface.jl) as a plugin, it would be easier to handle the manual cherry-pick.

Tokazama commented 3 years ago

Part of start up time is due to Requires. We're trying to resolve that by getting rid of it now that ArrayInterface.jl is better established. I don't have a strong opinion on how this transition should be managed within OffsetArays.jl, but we do need to move the OffsetArrays code here at some point.

johnnychen94 commented 3 years ago

Can we have a stub package ArrayInterfaceSubs as https://github.com/JuliaArrays/ArrayInterface.jl/issues/211#issuecomment-944028619 proposed? Until https://github.com/JuliaLang/Pkg.jl/issues/1285 is supported, this is the best option so far if we want to remove the Requires latency.

cc: @chriselrod

Tokazama commented 3 years ago

We've always planned on getting rid of Requires, so resolution of JuliaLang/Pkg.jl#1285 shouldn't make a difference.

timholy commented 3 years ago

I think we want ArrayInterface pretty broadly, and if it's going to be loaded by multiple packages then it won't have a direct impact on this package (it will already be loaded by something else).

timholy commented 3 years ago

@johnnychen94's concerns about potential divergence for testing Base are quite relevant. For 1.0 compatibility, I'd agree that it's a disadvantage until Julia 1.6 is officially declared LTS. Once that happens, I say we abandon any duty to maintain compat with 1.0.

johnnychen94 commented 3 years ago

I think we want ArrayInterface pretty broadly, and if it's going to be loaded by multiple packages then it won't have a direct impact on this package (it will already be loaded by something else).

I think this is where we diverges. My understanding of ArrayInterface is that it is still experimental and if anything that is stable enough it should goes to Base first. Letting every other community to adopt ArrayInterface really has the tendency to diverges from the Base Array interface especially when ArrayInterface itself doesn't have a solid documentation on how other packages should coordinate with it.

I can understand that LoopVectorization is built on top of ArrayInterface to extract enough information during compilation time, but really it is not clear from a non-ArrayInterface-maintainer's perspective what is needed to support LoopVectorization's magic turbo. If there is a clear interface documentation on "doing steps 1, 2, 3 and you'll get your array type supported by LoopVectorization" and then I'll just do it myself for every array types I know of.

We've always planned on getting rid of Requires, so resolution of JuliaLang/Pkg.jl#1285 shouldn't make a difference.

It makes differences. JuliaLang/Pkg.jl#1285 kills the debates of "which package is more lightweight than the other" and gives a solution that both parties are happy with. This PR simply moves latencies from ArrayInterface to OffsetArrays because some people think ArrayInterface ecosystem is more important than the OffsetArray ecosystem. I'm fine on having this dependency since it can be negligible if multiple packages are loaded but I still want to point out that the final solution is JuliaLang/Pkg.jl#1285 instead of persuading other packages to take the latency burden.

Once that happens, I say we abandon any duty to maintain compat with 1.0.

I'm okay with this.

Tokazama commented 3 years ago

My understanding of ArrayInterface is that it is still experimental and if anything that is stable enough it should goes to Base first.

The parts I've been working into this PR are very mature and the most unstable part of the whole package is the indexing protocol (which needs upstream adoption and testing to really identify potential problems). Some of these things would be breaking changes for Base, but much easier to transition into Base if they were adopted and tested in the wild first.

The caveat to this is that the Julia community may want to just wait until Julia 2.0 and handle all the breaking changes at once.

EDIT: I haven't had time yet to work out the test here, but the code I've implemented translates into support for getting axes and size if OffsetArray wraps an array with named dimensions and supports methods like known_length and known_size when a static array is wrapped.