Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add some more interfaces to BaseInterfaces.jl #27

Merged
merged 21 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 4 additions & 21 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
matrix:
version:
- '1.6'
- '1.7'
- '1'
- 'nightly'
os:
- ubuntu-latest
Expand All @@ -35,27 +35,10 @@ jobs:
- uses: julia-actions/cache@v1
- uses: julia-actions/julia-buildpkg@v1
- uses: julia-actions/julia-runtest@v1
- name: Run BaseInterfaces tests
run: julia --project=BaseInterfaces -e 'using Pkg; pkg"dev ."; pkg"update"; pkg"test"'
shell: bash
- uses: julia-actions/julia-processcoverage@v1
- uses: codecov/codecov-action@v2
with:
files: lcov.info
docs:
name: Documentation
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- uses: actions/checkout@v2
- uses: julia-actions/setup-julia@v1
with:
version: '1'
- uses: julia-actions/julia-buildpkg@v1
- uses: julia-actions/julia-docdeploy@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- run: |
julia --project=docs -e '
using Documenter: DocMeta, doctest
using Interfaces
DocMeta.setdocmeta!(Interfaces, :DocTestSetup, :(using Interfaces); recursive=true)
doctest(Interfaces)'
27 changes: 27 additions & 0 deletions .github/workflows/Documentation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: Documentation

on:
push:
branches:
- main # update to match your development branch (master, main, dev, trunk, ...)
tags: '*'
pull_request:

jobs:
build:
permissions:
contents: write
statuses: write
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: julia-actions/setup-julia@v1
with:
version: '1'
- name: Install dependencies
run: julia --project=docs/ -e 'using Pkg; pkg"dev ."; pkg"dev ./BaseInterfaces"; Pkg.instantiate()'
- name: Build and deploy
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # If authenticating with GitHub Actions token
DOCUMENTER_KEY: ${{ secrets.DOCUMENTER_KEY }} # If authenticating with SSH deploy key
run: julia --project=docs/ docs/make.jl
38 changes: 34 additions & 4 deletions BaseInterfaces/README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,36 @@
# BaseInterfaces

[![Stable](https://img.shields.io/badge/docs-stable-blue.svg)](https://rafaqz.github.io/BaseInterfaces.jl/stable/)
[![Dev](https://img.shields.io/badge/docs-dev-blue.svg)](https://rafaqz.github.io/BaseInterfaces.jl/dev/)
[![Build Status](https://github.com/rafaqz/BaseInterfaces.jl/actions/workflows/CI.yml/badge.svg?branch=main)](https://github.com/rafaqz/BaseInterfaces.jl/actions/workflows/CI.yml?query=branch%3Amain)
[![Coverage](https://codecov.io/gh/rafaqz/BaseInterfaces.jl/branch/main/graph/badge.svg)](https://codecov.io/gh/rafaqz/BaseInterfaces.jl)
[![Stable](https://img.shields.io/badge/docs-stable-blue.svg)](https://rafaqz.github.io/Interfaces.jl/stable/)
[![Dev](https://img.shields.io/badge/docs-dev-blue.svg)](https://rafaqz.github.io/Interfaces.jl/dev/)
[![Build Status](https://github.com/rafaqz/Interfaces.jl/actions/workflows/CI.yml/badge.svg?branch=main)](https://github.com/rafaqz/Interfaces.jl/actions/workflows/CI.yml?query=branch%3Amain)
[![Coverage](https://codecov.io/gh/rafaqz/Interfaces.jl/branch/main/graph/badge.svg)](https://codecov.io/gh/rafaqz/Interfaces.jl)

BaseInterfaces.jl is a subpackage of Interfaces.jl that provides predifined
definition and testing for Base Julia interfaces.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Currently this includes:
- A general iteration interface: `IterationInterface`
- `AbstractArray` interface: `ArrayInterface`
- `AbstractSet` interface: `SetInterface`
- `AbstractDict` interface: `DictInterface`


Testing your object follows the interfaces is as simple as:

```julia
using BaseInterfaces, Interfaces
Interfaces.tests(DictInterface, MyDict, [mydict1, mydict2, ...])
```

Declaring that it follows the interface is done with:

```julia
@implements DictInterface{(:optional, :components)} MyDict
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
```

Where components can be chosen from `Interfaces.optional_keys(DictInterface)`.

See [the docs](https://rafaqz.github.io/Interfaces.jl/stable/) for use.

If you want to add more Base julia interfaces here, or think the existing
ones could be improved, please make an issue or pull request.
32 changes: 30 additions & 2 deletions BaseInterfaces/src/BaseInterfaces.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,43 @@ module BaseInterfaces

using Interfaces

export IterationInterface
export ArrayInterface, DictInterface, IterationInterface, SetInterface

include("iteration.jl")
include("dict.jl")
include("set.jl")
include("array.jl")

# Some example interface delarations.
rafaqz marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would split the implementation declarations into another file (or several)

# @implements ArrayInterface Base.LogicalIndex # No getindex
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why is it an AbstractArray?

Copy link
Owner Author

@rafaqz rafaqz Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base just breaks the interface 😱

I have to special case it in DimensionalData.jl for this reason...

(The reason is getindex is very slow on LogicalIndex, but indexing only accepts AbstractArrray, Colon or Int so it "has to be" AbstractArray. Internally juloa uses iterate not getindex in indexing, so it still works)

@implements ArrayInterface UnitRange
@implements ArrayInterface StepRange
@implements ArrayInterface Base.Slice
@implements ArrayInterface Base.IdentityUnitRange
@implements ArrayInterface Base.CodeUnits
@implements ArrayInterface{(:setindex!,:similar_type,:similar_eltype,:similar_size)} Array
@implements ArrayInterface{(:setindex!,:similar_type,:similar_size)} BitArray
@implements ArrayInterface{(:setindex!,)} SubArray
@implements ArrayInterface{(:setindex!,)} PermutedDimsArray
@implements ArrayInterface{(:setindex!,)} Base.ReshapedArray

@implements DictInterface{(:setindex,)} Dict
@implements DictInterface{(:setindex,)} IdDict
@implements DictInterface{(:setindex,)} WeakKeyDict
@implements DictInterface Base.EnvDict
@implements DictInterface Base.ImmutableDict
# @implements DictInterface Base.Pairs - not on 1.6?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this version-specific?


@implements IterationInterface{(:reverse,:indexing,)} UnitRange
@implements IterationInterface{(:reverse,:indexing,)} StepRange
@implements IterationInterface{(:reverse,:indexing,)} Array
@implements IterationInterface{(:reverse,)} Base.Generator
@implements IterationInterface{(:reverse,:indexing,)} Tuple
@implements IterationInterface{(:reverse,)} Base.Generator

# TODO add grouping to reduce the number of options
@implements SetInterface{(:copy,:empty,:emptymutable,:hasfastin,:setdiff,:intersect,:empty!,:delete!,:push!,:copymutable,:sizehint)} Set
@implements SetInterface{(:copy,:empty,:emptymutable,:hasfastin,:setdiff,:intersect,:empty!,:delete!,:push!,:copymutable,:sizehint)} BitSet
@implements SetInterface{(:empty,:emptymutable,:hasfastin,:intersect,:union,:sizehint)} Base.KeySet

end
93 changes: 93 additions & 0 deletions BaseInterfaces/src/array.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
#= Abstract Arrays

Methods to implement Brief description
size(A) Returns a tuple containing the dimensions of A
getindex(A, i::Int) (if IndexLinear) Linear scalar indexing
getindex(A, I::Vararg{Int, N}) (if IndexCartesian, where N = ndims(A)) N-dimensional scalar indexing

Optional methods Default definition Brief description
IndexStyle(::Type) IndexCartesian() Returns either IndexLinear() or IndexCartesian(). See the description below.
setindex!(A, v, i::Int) (if IndexLinear) Scalar indexed assignment
setindex!(A, v, I::Vararg{Int, N}) (if IndexCartesian, where N = ndims(A)) N-dimensional scalar indexed assignment
getindex(A, I...) defined in terms of scalar getindex Multidimensional and nonscalar indexing
setindex!(A, X, I...) defined in terms of scalar setindex! Multidimensional and nonscalar indexed assignment
iterate defined in terms of scalar getindex Iteration
length(A) prod(size(A)) Number of elements
similar(A) similar(A, eltype(A), size(A)) Return a mutable array with the same shape and element type
similar(A, ::Type{S}) similar(A, S, size(A)) Return a mutable array with the same shape and the specified element type
similar(A, dims::Dims) similar(A, eltype(A), dims) Return a mutable array with the same element type and size dims
similar(A, ::Type{S}, dims::Dims) Array{S}(undef, dims) Return a mutable array with the specified element type and size

Non-traditional indices Default definition Brief description
axes(A) map(OneTo, size(A)) Return a tuple of AbstractUnitRange{<:Integer} of valid indices
similar(A, ::Type{S}, inds) similar(A, S, Base.to_shape(inds)) Return a mutable array with the specified indices inds (see below)
similar(T::Union{Type,Function}, inds) T(Base.to_shape(inds)) Return an array similar to T with the specified indices inds (see below)
=#

# And arbitrary new type for array values
struct ArrayTestVal
a::Int
end

# In case `eltype` and `ndims` have custom methods
# We should always be able to use these to mean the same thing
_eltype(::AbstractArray{T}) where T = T
_ndims(::AbstractArray{<:Any,N}) where N = N

array_components = (;
mandatory = (;
type = A -> A isa AbstractArray,
eltype = (
A -> eltype(A) isa Type,
A -> eltype(A) == _eltype(A),
),
ndims = (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a fallback definition already, no? IIRC ndims comes from having size defined already.

Copy link
Owner Author

@rafaqz rafaqz Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesnt it use the second type parameter of AbstractArray?

I just wanted to check that was not messed up in the implementation of a custom type. Maybe not totally useful

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but that's not an issue because you have to declare where in a subtype the second parameter of a parent type goes. E.g. it doesn't matter that I can swap the parameters around in my definition:

julia> struct MyArr{N,T} <: AbstractArray{T,N} end

julia> ndims(MyArr{2,Int})
2

A -> ndims(A) isa Int,
A -> ndims(A) == _ndims(A),
),
size = (
A -> size(A) isa NTuple{<:Any,Int},
A -> length(size(A)) == ndims(A),
),
getindex = (
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
A -> A[begin] isa eltype(A),
A -> A[map(first, axes(A))...] isa eltype(A),
),
indexstyle = A -> IndexStyle(A) in (IndexCartesian(), IndexLinear()),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't mandatory - there is a default for this.

Copy link
Owner Author

@rafaqz rafaqz Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it will still always work? Its just saying its mandatory that it returns one of those options.

),
# TODO implement all the optional conditions
optional = (;
setindex! = (
A -> length(A) > 1 || throw(ArgumentError("Test arrays must have more than one element to test setindex!")),
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
A -> begin
# Tests setindex! by simply swapping the first and last elements
x1 = A[begin]; x2 = A[end]
A[begin] = x2
A[end] = x1
A[begin] == x2 && A[end] == x1
end,
A -> begin
fs = map(first, axes(A))
ls = map(last, axes(A))
x1 = A[fs...];
x2 = A[ls...]
A[fs...] = x2
A[ls...] = x1
A[fs...] == x2 && A[ls...] == x1
end,
),
similar_type = A -> similar(A) isa typeof(A),
similar_eltype = A -> begin
A1 = similar(A, ArrayTestVal)
eltype(A1) == ArrayTestVal && _wrappertype(A) == _wrappertype(A1)
end,
similar_size = A -> begin
A1 = similar(A, (2, 3))
size(A1) == (2, 3) && _wrappertype(A) == _wrappertype(A1)
end,
)
)

_wrappertype(A) = Base.typename(typeof(A)).wrapper

@interface ArrayInterface AbstractArray array_components "Base Julia AbstractArray interface"
17 changes: 17 additions & 0 deletions BaseInterfaces/src/dict.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

@interface DictInterface AbstractDict (
mandatory = (;
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
iterate = a -> Interfaces.test(IterationInterface, a.d; show=false) && first(iterate(a.d)) isa Pair,
eltype = a -> eltype(a.d) <: Pair,
getindex = a -> a.d[first(keys(a.d))] === last(first(iterate(a.d))),
),
optional = (;
setindex = a -> begin
a.d[a.k] = a.v
a.d[a.k] == a.v
end,
)
) """
`AbstractDict` interface requires Arguments, with `d = the_dict` mandatory, and
when `setindex` is needed, `k = any_valid_key_not_in_d, v = any_valid_val`
"""
19 changes: 8 additions & 11 deletions BaseInterfaces/src/iteration.jl
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function test_iterate(x)
end

# :size demonstrates an interface condition that instead of return a Bool,
function test_size(x)
function test_iterator_size(x)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use dispatch instead of if-else?

sizetrait = Base.IteratorSize(typeof(x))
if sizetrait isa Base.HasLength
length(x) isa Integer
Expand All @@ -49,9 +49,10 @@ function test_size(x)
end
end

function test_eltype(x)
function test_iterator_eltype(x)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use dispatch instead of if-else?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored all of this to be inline instead, I think having the function outside its harder to understand the interface

eltypetrait = Base.IteratorEltype(x)
if eltypetrait isa Base.HasEltype
x1, _ = iterate(x)
typeof(first(x)) <: eltype(x)
elseif eltypetrait isa Base.EltypeUnknown
true
Expand All @@ -60,11 +61,6 @@ function test_eltype(x)
end
end

#=
:indexing returns three condition functions.
We force the implementation of `firstindex` and `lastindex`
Or it is hard to test `getindex` generically
=#
function test_indexing(x)
firstindex(x) isa Integer &&
lastindex(x) isa Integer &&
Expand All @@ -74,18 +70,19 @@ end
# `Iterators.reverse` gives reverse iteration
test_reverse(x) = collect(Iterators.reverse(x)) == reverse(collect(x))

@interface IterationInterface (
@interface IterationInterface Any (
# Mandatory conditions: these must be met by all types
# that implement the interface.
mandatory = (
iterate = test_iterate,
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
size = test_size,
eltype = test_eltype,
isiterable = x -> Base.isiterable(typeof(x)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find it in the docs, where is it?

Copy link
Owner Author

@rafaqz rafaqz Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes its not, but it should be. Its in the tests to make people implement it in future.

The docs are unfortunately not a reliable source of truth.

size = test_iterator_size,
eltype = test_iterator_eltype,
),
# Optional conditions. These should be specified in the
# interface type if an object implements them: IterationInterface{(:reverse,:indexing)}
optional = (
reverse = test_reverse,
indexing = test_indexing,
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
)
)
) "An interface for Base Julia iteration"
60 changes: 60 additions & 0 deletions BaseInterfaces/src/set.jl
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# See https://github.com/JuliaLang/julia/issues/34677

# Requirements for AbstractSet subtypes:

set_components = (;
mandatory = (;
eltype = "elements eltype of set `s` are subtypes of `eltype(s)`" => s -> typeof(first(iterate(s))) <: eltype(s),
length = "set defines length and test object has length larger than zero" => s -> length(s) isa Int && length(s) > 0,
iteration = "follows the IterationInterface" => x -> Interfaces.test(IterationInterface, x; show=false),
in = "`in` is true for elements in set" => s -> begin
all(x -> in(x, s), s)
end,
),
optional = (;
copy = s -> begin
s1 = copy(s)
s1 !== s && s1 isa typeof(s) && collect(s) == collect(s1)
end,
empty = "returns a empty set able to hold elements of type U" => s -> begin
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
s1 = Base.empty(s)
eltype(s1) == eltype(s) || return false
s1 = Base.empty(s, Int)
eltype(s1) == Int || return false
end,
emptymutable = "returns a empty set able to hold elements of type U" => s -> begin
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
s1 = Base.empty(s)
s1 isa typeof(s)
eltype(s1) == eltype(s) || return false
s1 = Base.empty(s, Int)
eltype(s1) == Int || return false
end,
hasfastin = s -> Base.hasfastin(s) isa Bool,
setdiff = s -> begin
sd = setdiff(s, collect(s))
typeof(sd) == typeof(s) && length(sd) == 0
end,
intersect = s -> intersect(s, s) == s, # TODO
union = s -> union(s, s) == s, # TODO
empty! = "empty! removes all elements from the set" => s -> length(empty!(s)) == 0,
delete! = "delete! removes element valued x of the set" => s -> begin
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
x = first(iterate(s))
!in(delete!(s, x), x)
end,
push! = "push! adds element x to the set" => s -> begin
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
# TODO do this without delete!
x = first(iterate(s))
delete!(s, x)
push!(s, x)
in(x, s)
end,
copymutable = s -> begin
s1 = Base.copymutable(s)
s1 isa typeof(s) && s1 !== s
end,
# TODO is there anything we can actually test here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not, since there is no capacity function in Base :/

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its so weird that the hint just disappears and we cant access it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's just a hint/suggestion, not a requirement 🤷

sizehint = s -> (sizehint!(s, 10); true),
)
)

@interface SetInterface AbstractSet set_components "The `AbstractSet` interface"
Loading