Skip to content

Conversation

Tokazama
Copy link
Member

These don't need StaticInt and are semantically related to
AbstractDevice types in ArrayInterfaceCore:

  • defines_strides
  • device
  • stride_preserving_index

These don't need `StaticInt` and are semantically related to
`AbstractDevice` types in ArrayInterfaceCore:

* `defines_strides`
* `device`
* `stride_preserving_index`
@chriselrod
Copy link
Collaborator

How do we decide if something belongs in ArrayInterface or ArrayInterfaceCore?

@Tokazama
Copy link
Member Author

Tokazama commented Jul 29, 2022

How do we decide if something belongs in ArrayInterface or ArrayInterfaceCore?

I think we were originally just putting whatever didn't require StaticInt in ArrayInterfaceCore.jl. This one makes sense because we already have AbstractDevice and all its subtypes there. So if we shouldn't have this here perhaps we should move AbstractDevice.

@ChrisRackauckas
Copy link
Member

But, functions in ArrayInterfaceCore shouldn't need ArrayInterface to actually work. That would make them unreliable. So yes, it's things don't require Static and don't have dispatches on Base types that require Static

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #338 (59f479f) into master (6a4dff1) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
- Coverage   90.11%   90.06%   -0.05%     
==========================================
  Files           9        9              
  Lines        1345     1319      -26     
==========================================
- Hits         1212     1188      -24     
+ Misses        133      131       -2     
Impacted Files Coverage Δ
src/ArrayInterface.jl 85.32% <ø> (-0.75%) ⬇️
src/stridelayout.jl 89.51% <ø> (-0.09%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@Tokazama
Copy link
Member Author

But, functions in ArrayInterfaceCore shouldn't need ArrayInterface to actually work. That would make them unreliable. So yes, it's things don't require Static and don't have dispatches on Base types that require Static

Sometimes the line is bit blurry. #335 is an effort to disentangle the bulk of methods that don't actually require Static.jl but use it under the hood. A lot of methods aren't included in ArrayInterfaceCore because they accept a dim argument and we have traditionally used StaticInt to avoid type instabilities that may cause. In such cases I'm not sure if the method should be moved to ArrayInterfaceCore and we just define foo(x, ::StaticInt{N}) where {N} = foo(x, N) in ArrayInterface. A lot of those were developed before v1.6 was LTS, so we may have better inference now and not even need StaticInt for those methods.

So I've been trying to just chip away at this bit by bit, starting with cases like this where StaticInt isn't part of it anyway.

@Tokazama
Copy link
Member Author

Tokazama commented Aug 2, 2022

Was the consensus that this doesn't belong in ArrayInterfaceCore?

@ChrisRackauckas
Copy link
Member

Sometimes the line is bit blurry. #335 is an effort to disentangle the bulk of methods that don't actually require Static.jl but use it under the hood. A lot of methods aren't included in ArrayInterfaceCore because they accept a dim argument and we have traditionally used StaticInt to avoid type instabilities that may cause. In such cases I'm not sure if the method should be moved to ArrayInterfaceCore and we just define foo(x, ::StaticInt{N}) where {N} = foo(x, N) in ArrayInterface. A lot of those were developed before v1.6 was LTS, so we may have better inference now and not even need StaticInt for those methods.

That kind of piracy would change the semantics of the default method depending on whether you have only core or not. That leads to spooky action at a distance and shouldn't be done (because then the default behavior can change if someone else brings in ArrayInterface, which is likely). So we should stick to where there's a true full separation.

Also note that there was a bit of a practical definition there. Almost all of the uses of ArrayInterface used 0 of the Static methods, but >99% of the load time comes from the Static methods. So it just made sense to fix load times in SciML, Flux, etc. by just making the Static stuff a separate package. In some sense, it's still a bit odd that the two are together, or that it's not ArrayInterfaceStatic built on ArrayInterface, but I don't have to care about that because if it doesn't effect most packages using ArrayInterface then it's not my problem.

Most of the proposed static uses can probably work just fine using effects in the right way.

@Tokazama
Copy link
Member Author

Tokazama commented Aug 2, 2022

Perhaps it would make sense to instead define foo(x, dim::Number) = foo(x)[dim] where there would otherwise be no need for Static.jl.

This PR doesn't have that issue though. There's no need for StaticInt for device to work.

@ChrisRackauckas ChrisRackauckas merged commit bf07fa9 into master Aug 2, 2022
@ChrisRackauckas ChrisRackauckas deleted the defines_strides branch August 2, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants