ArrayInterface.jl icon indicating copy to clipboard operation
ArrayInterface.jl copied to clipboard

Array Interface Test Suite

Open Tokazama opened this issue 3 years ago • 5 comments

I've come across comments that it takes many months or years before a new array type is dependable because it takes time to catch all the corner cases and bugs. I think we should be able to get an array to be safely usable for common applications in a much shorter time. It might help if we put together a set of tests for common assumptions that an array should meet so that testing is easier. For example...

function test_array_indices(x)
    lininds = eachindex(IndexLinear(), x)
    carinds = CartesianIndices(axes(x))
    for (li,ci) = zip(lininds,carinds)
        @test x[li] == x[ci]
    end
    @test length(lininds) == length(carinds) == length(x) == prod(size(x))
end

We could make that example a bit more verbose and inject some logging so that it's clear where errors come from.

We could also do similar things with stride-layout stuff.

Tokazama avatar Jul 13 '22 18:07 Tokazama

I just registered this if you want to try it out. Structured interface tests with traits for free:

https://github.com/rafaqz/Interfaces.jl

Also let's you split an interface into components, e.g. making setindex! optional.

Theres an implementation of IterationInterface in the BaseInterfaces.jl subpackage, which isn't registered yet. It might give more detail than the readme demo.

rafaqz avatar Jul 13 '22 21:07 rafaqz

That's definitely interesting and there seems to be a consistent request for some way of guaranteeing that a new type provides valid support for an interface. Reading through the examples, it seems a lot more complex than just implementing a basic test suite composed of clearly documented functions. It would help if you could give an example of how this could be implemented for the multidimensional indices interface.

Tokazama avatar Jul 13 '22 23:07 Tokazama

@interface ArrayInterface (
    mandatory = (
        size = x -> length(x) == prod(size(x)),
        indices = (
            x -> length(eachindex(IndexLinear(), x)) == length(x),
            x -> length(CartesianIndices(axes(x))) == length(x),
            x -> map((l, c) -> x[l] == x[c], CartesianIndices(axes(x)), eachindex(IndexLinear(), x)) 
        ) 
    ),
    optional = (
        setindex! = x -> false # need to write these...,
        broadcast = x -> false,
    )
)

Then in a package

julia> @implements ArrayInterface Array [zeros(10, 10)]

true

And we have traits:

julia> Interfaces.implements(ArrayInterface, Array)
true

julia> Interfaces.implements(ArrayInterface{:setindex!}, Array)
false

Of course we just haven't written setindex! properly yet, but we also didn't specify that Array implements it.

rafaqz avatar Jul 14 '22 09:07 rafaqz

Pretty much any of this can be changed if you have better ideas, the point was to get something started quickly - and returning NamedTuple of functions/functors for tests is very easy to write for when you need some of it to be modular.

Edit: You can also organise that code however you like - the functions could be written separately. The macro doesn't do anything with the NamedTuple except use it as an object.

E.g:

function test_array_indices(x)
    lininds = eachindex(IndexLinear(), x)
    carinds = CartesianIndices(axes(x))
    for (li,ci) = zip(lininds,carinds)
        @test x[li] == x[ci]
    end
    return length(lininds) == length(carinds) == length(x) == prod(size(x))
end

@interface ArrayInterface (mandatory=(indices=test_array_indices,), optional=())

I need to accept Test.Pass as true in Interfaces.jl then your code would work as-is. And maybe also allow skipping the mandatory/optional distinction when there are only mandatory components.

rafaqz avatar Jul 14 '22 09:07 rafaqz

The examples are really helpful for understanding how to translate this. I'll make an issue with some more questions in the Interface.jl repo so this one can stay on topic for other comments about tests necessary for this to work.

Tokazama avatar Jul 14 '22 19:07 Tokazama