add some more interfaces to BaseInterfaces.jl #27
Conversation
Codecov Report
@@ Coverage Diff @@
## main #27 +/- ##
==========================================
- Coverage 86.20% 82.38% -3.82%
==========================================
Files 5 5
Lines 145 159 +14
==========================================
+ Hits 125 131 +6
- Misses 20 28 +8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
rafaqz
force-pushed
the
more_interfaces
branch
from
addcfef to
fab1adc
Compare
rafaqz
changed the title
|
Wow, large effort! I'll try to review it in the coming days. Just wondering, did you check out other initiatives like https://github.com/JuliaArrays/ArrayInterface.jl? |
|
Yeah, I've been following ArrayInterface for some years. But ArrayInterface.jl really isn't the same as the base
(emphasis mine) The idea in BaseInterfaces.jl is just to have a simple definitions of Base Julia interfaces as far as they are documented, or as used in Base where they are not documented. Its a test of Interfaces.jl capacity to represent a range of interfaces, and should be useful for people extending theses interfaces to have a predefined test suit for them. I'm not sure that the Of course, there will be some overlap in terms of traits as ArrayInterface.jl does include some traits for simple base interface things as well. |
rafaqz
force-pushed
the
more_interfaces
branch
from
931b8be to
e7e5c81
Compare
|
Feel free to ping me when this is more or less ready for review! |
|
Any time really, feedback would be good. It will be a work in progress untul we register, so we can merge PRs as we go. |
|
Okay cool, I'll try to take a look |
BaseInterfaces/src/array.jl
Outdated
| A -> A[begin] isa eltype(A), | ||
| A -> A[map(first, axes(A))...] isa eltype(A), | ||
| ), | ||
| indexstyle = A -> IndexStyle(A) in (IndexCartesian(), IndexLinear()), |
There was a problem hiding this comment.
This isn't mandatory - there is a default for this.
There was a problem hiding this comment.
So it will still always work? Its just saying its mandatory that it returns one of those options.
BaseInterfaces/src/set.jl
Outdated
| s1 = Base.copymutable(s) | ||
| s1 isa typeof(s) && s1 !== s | ||
| end, | ||
| # TODO is there anything we can actually test here? |
There was a problem hiding this comment.
Unfortunately not, since there is no capacity function in Base :/
There was a problem hiding this comment.
Its so weird that the hint just disappears and we cant access it.
There was a problem hiding this comment.
Well it's just a hint/suggestion, not a requirement 🤷
| A -> eltype(A) isa Type, | ||
| A -> eltype(A) == _eltype(A), | ||
| ), | ||
| ndims = ( |
There was a problem hiding this comment.
This should have a fallback definition already, no? IIRC ndims comes from having size defined already.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I see that @Seelengrab has reviewed the array, dict and set interfaces, so I'll submit my review on everything except those three files for now
| @@ -0,0 +1,12 @@ | |||
| # BaseInterfaces reference | |||
There was a problem hiding this comment.
This looks like a failed .md file
| @test Interfaces.test(ArrayInterface, Base.Slice, [Base.Slice(100:150)]) | ||
| @test Interfaces.test(ArrayInterface, Base.IdentityUnitRange, [Base.IdentityUnitRange(100:150)]) | ||
| @test Interfaces.test(ArrayInterface, Base.CodeUnits, [codeunits("abcde")]) | ||
| # No `getindex` defined for LogicalIndex |
There was a problem hiding this comment.
This method will also fail for GPU arrays. Maybe it should be an optional part of the interface?
There was a problem hiding this comment.
It only fails with e.g. CUDA.allowscalar(true) on GPU. So thst would need to be false.
The docs really specificially say its one of the main mandatory methods, I kinda want the failed test there to demonstrate the problem...
|
Thanks for the feedback, I've implemented most of these and a bunch more. Also fixed up testing and display a lot and accidentally merged it here! But will merge as-is anyway and more can be added in another PR. |
This PR adds the start of
AbstractSet,AbstractDict, andAbstractArrayto BaseInterfaces.jlHopefully this will end with interface tests that help people write interfaces for these base types and
know they are correct and complete.
@gdalle thoughts or contributions to the cause would be appreciated!