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 parenttype method #42923

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add parenttype method #42923

wants to merge 4 commits into from

Conversation

Tokazama
Copy link
Contributor

@Tokazama Tokazama commented Nov 3, 2021

This is a fairly simple method that has proven to be a critical component for a lot of methods in ArrayInterface.jl. It seems an appropriate companion to the current methods that compose the AbstractArray interface in base.

@ianatol ianatol added arrays [a, r, r, a, y, s] feature Indicates new feature / enhancement requests labels Nov 3, 2021
@fredrikekre
Copy link
Member

fredrikekre commented Nov 3, 2021

Can you describe how this would/should be used and why this is useful? I couldn't really understand this from a brief look at ArrayInterface.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 3, 2021

A simple example is device where we simply want to know if an array is backed by memory that can produce a pointer. Wrapping arrays doesn't change this so it's obnoxious to require that all new array types write a new device method.

@chriselrod and @ChrisRackauckas probably use it frequently too, in case they want to vouch for this.

@chriselrod
Copy link
Contributor

Type wrappers are hard to work with if you wish to understand the memory layout of an array.
parent_type makes this simpler, e.g. consider some of the code implementing contiguous_axis, which is used for describing which of the axis contains contigous memory:

"""
    contiguous_axis(::Type{T}) -> StaticInt{N}
Returns the axis of an array of type `T` containing contiguous data.
If no axis is contiguous, it returns a `StaticInt{-1}`.
If unknown, it returns `nothing`.
"""
contiguous_axis(x) = contiguous_axis(typeof(x))
contiguous_axis(::Type{<:StrideIndex{N,R,C}}) where {N,R,C} = static(C)
contiguous_axis(::Type{<:StrideIndex{N,R,nothing}}) where {N,R} = nothing
function contiguous_axis(::Type{T}) where {T}
    if parent_type(T) <: T
        return nothing
    else
        return contiguous_axis(parent_type(T))
    end
end
contiguous_axis(::Type{<:Array}) = One()
contiguous_axis(::Type{<:BitArray}) = One()
contiguous_axis(::Type{<:AbstractRange}) = One()
contiguous_axis(::Type{<:Tuple}) = One()
function contiguous_axis(::Type{T}) where {T<:VecAdjTrans}
    c = contiguous_axis(parent_type(T))
    if c === nothing
        return nothing
    elseif c === One()
        return StaticInt{2}()
    else
        return -One()
    end
end
function contiguous_axis(::Type{T}) where {T<:MatAdjTrans}
    c = contiguous_axis(parent_type(T))
    if c === nothing
        return nothing
    elseif isone(-c)
        return c
    else
        return StaticInt(3) - c
    end
end
function contiguous_axis(::Type{T}) where {T<:PermutedDimsArray}
    c = contiguous_axis(parent_type(T))
    if c === nothing
        return nothing
    elseif isone(-c)
        return c
    else
        return from_parent_dims(T, c)
    end
end

Not shown is code for SubArray, which works similarly but then needs to update the contiguous axis for whether it has been removed (non-AbstractUnitRange index) or shifted (leading axis removed).

By using parent_type in this manner, contiguous_axis is able to work on a transpose of a PermutedDimsArray of a SubAray of an Adjoint...
LoopVectorization needs information like this on the relative ranking of strides, which axes(s) are contiguous in memory, etc.

@chriselrod and @ChrisRackauckas probably use it frequently too

That said, these wrapper types are annoying to deal with, so my array packages tend to canonicalize strided arrays to a StrideArray, which flattens everything into a single expressive array family, making it much easier to work with.
So I don't believe I've used it much inside ArrayInterface.jl, where all the handling/support for wrappers is concentrated.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

@fredrikekre, I mirrored the documentation for parent since it is very similar in definition and application, but if there is something else you think is unclear I'd be happy to add it.

@KristofferC KristofferC requested a review from mbauman November 5, 2021 15:52
@KristofferC
Copy link
Member

KristofferC commented Nov 5, 2021

I don't see how you can use parent / parenttype in any generic code. The "wrapped" array is only a part of what makes the AbstractArray. There might be additional data in there (like in a SubArray), there might be type parameters that are important.

How can you do anything with the return value from parent on an arbitrary AbstractArray? Like

julia> parent(A)
3×3 Matrix{Float64}:
 0.867555  0.833865  0.671841
 0.78186   0.954977  0.16845
 0.901881  0.855753  0.398882

Ok, thanks, so what does that tell me about A?

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

Is this actually helpful? The parent function is one of my biggest pet peeves in all of base Julia — it has the appearance of being useful for generic code but is actually impossible to use generically. Last I reviewed its use, it was not once used generically. Edit: @chriselrod's clever parent_type(T) <: T || recurse(parent_type(T)) strategy is the first use I've seen that actually does something with parent_type without knowing what T is... but note that it requires lots of non-generic specializations to actually be useful.

That's not to say there isn't a pain point for wrapped arrays — there definitely is — but I really don't see parent (or its type) as being a part of that solution. I'd love to be proven wrong!

I'd favor an approach like Chris suggests: canonicalizing wrapped arrays to a standard format (like strided or sparse or the like).

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

Is this actually helpful? The parent function is one of my biggest pet peeves in all of base Julia — it has the appearance of being useful for generic code but is actually impossible to use generically. Last I reviewed its use, it was not once used generically.

In practice the previously mentioned device method looks something like this

device(::Type{T}) where {T<:Array} = CPUPointer()
function device(::Type{T}) where {T<:AbstractArray}
    if parenttype(T) <: T
        return UnkownDevice()
    else
        return device(parenttype(T))
    end
end

So we can still pull meaningful information out of an array's parent type without knowing the type of the most superficial layer.

I'd favor an approach like Chris suggests: canonicalizing wrapped arrays to a standard format (like strided or sparse or the like).

Is the alternative proposal here to have something like

struct WrappedArray{T,N,P,M} <: AbstractArray{T,N}
    parent::P
    metadata::M
end

...so that we don't need parent or parenttype?

If we could replace all the current array implementations with something like WrappedArray that'd be fine, but that seems a lot more complicated than what I'm proposing.

Alternatively, if the issue is generalizability then perhaps buffer and buffertype would be more appropriate. Other than that, I'm not sure how much more general this can be.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

The point is that we don't really define what parent means — for example that contiguous_axis definition will be wrong for a custom wrapper that happily defines parent but shuffles its indices.

You simply don't know how to interpret parent or parent_type without knowing what the wrapper does with that parent.

And no, I'm not suggesting that WrappedArray — rather, my harebrained idea is to ask for an array's implementation. This may return a recomputed implementation of a strided array or a sparse array or an "opaque" array where you know nothing or ... that could allow for downstream generic code.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

I've always thought that parent was for returning the array's immediately nested reference to where its elements are stored.
If this syntax or definition is unclear then better documentation or alternative method names is fine.
However, it's still useful to have a generic method for grabbing an arrays immediately wrapped source of memory.
I agree that most of the time we need additional context concerning what the wrapper type does to a parent, but that doesn't negate the utility of a generic way of referring to parent data.

For example, composing a canonical StrideArray type from nested arrays can be implemented through composing a StrideIndex through a process like this:

StrideArray(x::DenseArray) = StrideArray(x, StrideIndex(x)) 
function StrideArray(x::AbstractArray)
    p = parent(x)
    x === p || error("Cannot convert $x to StrideArray")
    # transform StrideArray by index transformations specific to x
    transform_strides(StrideIndex(p), ArrayIndex(x))
end

This probably isn't the most optimal way to do this, but it still demonstrates that having a standard way of referring to the parent memory is still useful if we don't know the type of x.
In this instance it clearly requires more context (the output of ArrayIndex), but that doesn't mean you need to know what x is.

@mcabbott
Copy link
Contributor

mcabbott commented Nov 5, 2021

If this device query is to know whether you can call BLAS-like routines, the next step is probably going to be to call strides or pointer. Something like #30432 seems like a really neat way to allow that to be a safe query about the parent:

julia> pointer(rand(3)')  # works through a wrapper
Ptr{Float64} @0x00007f24e8dee4d0

julia> pointer(cu(rand(3)))  # different type
CuPtr{Float32}(0x0000000cf6000400)

julia> pointer(1:3)  # maybe this should be `nothing`
ERROR: conversion to pointer not defined for UnitRange{Int64}

That works on an instance, not the type. Would it be too weird to just add methods something like these?

Base.strides(::Type{<:Matrix}) = (1, missing)
Base.strides(::Type{<:AbstractMatrix}) = (missing, missing)
Base.strides(::Type{<:Adjoint{<:Any, P}}) where P<:AbstractMatrix = reverse(strides(P))

Base.pointer(::Type{T}) where T = Base._return_type(pointer, Tuple{T})

In some cases, you could probably use inference to get the value, if the wrapper's author defined the method for an instance. And if they did not define pointer, then you cannot use the fact that they wrap an Array anyway. That seems like a way to avoid the broken state where someone does define parent_type but does not define pointer.

@chriselrod
Copy link
Contributor

chriselrod commented Nov 5, 2021

I don't see how you can use parent / parenttype in any generic code. The "wrapped" array is only a part of what makes the AbstractArray. There might be additional data in there (like in a SubArray), there might be type parameters that are important.

How can you do anything with the return value from parent on an arbitrary AbstractArray? Like

julia> parent(A)
3×3 Matrix{Float64}:
 0.867555  0.833865  0.671841
 0.78186   0.954977  0.16845
 0.901881  0.855753  0.398882

Ok, thanks, so what does that tell me about A?

This is less a tool meant to be used generically, and more of a tool for peeling through wrappers.
E.g, in the earlier example:

function contiguous_axis(::Type{T}) where {T<:MatAdjTrans}
    c = contiguous_axis(parent_type(T))
    if c === nothing
        return nothing
    elseif isone(-c)
        return c
    else
        return StaticInt(3) - c
    end
end

Your example tells us that axis 2 of A is contiguous.

This means every new wrapper type has to implement methods indicating how they transform the properties of interest. For example, the Adjoint wrapper needs to say it swaps the order of strides.
Then, if each wrapper implements this, a query on the ranking of strides (or, as in that example, on which stride is contiguous), the query will work on arbitrary nestings.

The primary use case is to facilitate introspection, for those who want to support it.

Anyone implementing parent_type should implement the other methods (like contiguous_axis) too, if it has any impact on them.

@KristofferC
Copy link
Member

There is nothing in the docs for the interface of AbstractArray that talks about any wrapping of other arrays. It is defined by methods. For example:

struct FArray{T, N} <: AbstractArray{T, N}
    f::Function
    size::NTuple{N, Int}
end

Base.getindex(F::FArray{T, N}, I::Vararg{Int, N}) where {N, T} = F.f(I...)::T
Base.size(f::FArray) = f.size

f = FArray{Float64, 2}((x,y) -> sin(x)*cos(y), (2,3))

is a fine AbstractArray. What should parent be? Or you can have an array that wraps two other arrays and picks one element from one or the other based on the index. Etc etc. There's just no way getting around that you are using some extra knowledge about the array than just the return value of parent (and the rest of the values from the AbstractArray interface) as soon as you get something useful out of it.

@chriselrod
Copy link
Contributor

chriselrod commented Nov 5, 2021

That's not to say there isn't a pain point for wrapped arrays — there definitely is — but I really don't see parent (or its type) as being a part of that solution. I'd love to be proven wrong!

I'd favor an approach like Chris suggests: canonicalizing wrapped arrays to a standard format (like strided or sparse or the like).

In the Julia package ecosytem, parent_type can be part of the solution by giving us a way to perform the canonicalization through wrappers.
But I also would favor us canonicalizing things here in the first place, obviating the need for peeling through wrappers.

EDIT:
parent_type of FArray{T,N} would be FArray{T,N} / it would be the end of the line.
You would therefore have to define all the quering functions directly on it.
This would then let someone run these queries on Adjoint{T,FArray{T,N}}, or a SubArray of that adjoint.

There's just no way getting around that you are using some extra knowledge about the array than just the return value of parent

Yes, unfortunately there is no way around this.
Like in my example, specific methods had to be added for each query for each wrapper and each base type.

The benefit is that it makes wrappers more composable. It's a means around exponential explosion in number of possible type combinations from nested wrappers.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

Chris's example is a good case where the syntax itself provides more succinct code (although not strictly necessary to get the parent of Adjoint or Transpose).
I provided a use case where access to the parent array can be used generically if proper context is provided through another method.
At its worst, the utility of parent and parenttype is convenient syntax for what can be done otherwise.
If used with proper context (which I reiterate is not dependent on known the wrapping array type if used properly), then it can help solve a lot of problems with wrapper arrays.
I'm in favor of better support for a canonical stride array, but that does not negate the utility of this method and implementing it may event benefit from this.

@mcabbott
Copy link
Contributor

mcabbott commented Nov 5, 2021

Anyone implementing parent_type should implement the other methods (like contiguous_axis) too, if it has any impact on them.

But then what's gained by parent_type over just implementing contiguous_axis (or strides)? It seems to offer opportunities for things to go wrong. Concrete example:

julia> using TransmuteDims

julia> parenttype(::Type{<:TransmutedDimsArray{T,N,I1,I2,A}}) where {T,N,I1,I2,A} = A;  
  # same as PermutedDimsArray

julia> transmute([1 2 3; 4 5 6], (3,2,1))
1×3×2 transmute(::Matrix{Int64}, (0, 2, 1)) with eltype Int64:
[:, :, 1] =
 1  2  3

[:, :, 2] =
 4  5  6

julia> contiguous_axis(typeof(ans))
One()

If the fallback was strides(_) = nothing, you would safely conclude that you do not actually know anything about this type.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

The primary use case is to facilitate introspection, for those who want to support it.

But it's not really doing that in any meaningful way. In your example:

function contiguous_axis(::Type{T}) where {T<:MatAdjTrans}
    c = contiguous_axis(parent_type(T))
    # ...

this could just as easily be:

function contiguous_axis(::Type{MatAdjTrans{<:Any, P}}) where {P}
    c = contiguous_axis(P)
    # ...

This is what I mean by needing to know about the wrapper in order to use the function. You already know the answer. It's not gaining you anything.

parent_type can be part of the solution

This is where I disagree. I think it's a trap.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

The primary use case is to facilitate introspection, for those who want to support it.

But it's not really doing that in any meaningful way. In your example:

function contiguous_axis(::Type{T}) where {T<:MatAdjTrans}
    c = contiguous_axis(parent_type(T))
    # ...

this could just as easily be:

function contiguous_axis(::Type{MatAdjTrans{<:Any, P}}) where {P}
    c = contiguous_axis(P)
    # ...

This is what I mean by needing to know about the wrapper in order to use the function. You already know the answer. It's not gaining you anything.

I don't understand how this is any different than what I said. In this case it is nice syntax when you don't want to write out all the other parameters of a type to get to the parent type. Not necessary but sometimes nice.

I'm not sure how it's a trap. I gave an example where the parent buffer can safely be accessed if we also grab the index transformation. If you're going to use parent and what you grab from it depends on index transformations then have another method for grabbing that so you know what you're assuming.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

Thinking a bit more on the general strategies of parent_type(T) <: T || recurse(parent_type(T)) or parent(A) === A || recurse(parent(A)), I don't think they're valid without knowing what the wrapper is doing. For information about strides, you need to know how the wrapper treats indices. For information about sparsity, you need to know how the wrapper is mapping values. Maaaybe the device one works, but again, you really need to know what's happening in the wrapper to actually generate the code.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

Thinking a bit more on the general strategies of parent_type(T) <: T || recurse(parent_type(T)) or parent(A) === A || recurse(parent(A)), I don't think they're valid without knowing what the wrapper is doing. For information about strides, you need to know how the wrapper treats indices. For information about sparsity, you need to know how the wrapper is mapping values. Maaaybe the device one works, but again, you really need to know what's happening in the wrapper to actually generate the code.

Yeah, and depending on what we need to do with the parent data you can have another method inquiring about that. I admit that there's a limit to how useful blindly recursing through the layers of an array is. That doesn't mean we can't have other methods to guide us while traversing nested arrays.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

Yeah, and depending on what we need to do with the parent data you can have another method inquiring about that

Yes, exactly! That's the problem. It's that you can't just implement parent and expect things to work. You also need to implement methods for all functions in which parent is used. Be they in ArrayInterface.jl or StridedArrays.jl or BlockArrays.jl or SparseArrays.jl or .... In fact, if you implement parent without implementing those secondary methods, your array will be broken by default.

That's the trap. It feels like a solution, but there's this other part that you need to know about.

@chriselrod
Copy link
Contributor

chriselrod commented Nov 5, 2021

This is what I mean by needing to know about the wrapper in order to use the function. You already know the answer. It's not gaining you anything.

parent_type can be part of the solution

This is where I disagree. I think it's a trap.

Examples from ArrayInterface's tests:

    @test @inferred(contiguous_axis(@view(PermutedDimsArray(A,(3,1,2))[2:3,2,:])')) === ArrayInterface.StaticInt(-1)
    @test @inferred(contiguous_axis(@view(PermutedDimsArray(A,(3,1,2))[:,1:2,1])')) === ArrayInterface.StaticInt(1)

E.g., Adjoints of SubArrays of PermutedDimsArrays.
Yes, we had to know about each of the three.
But we did not have to know about the combination.

To re-emphasize what I said earlier:
You always do need to know what the wrappers are doing.

The point is to avoid having to know about what the combinations are doing, letting you treat them individually.

It makes the number of method definitions needed linear, rather than exponential.

If the fallback was strides(_) = nothing, you would safely conclude that you do not actually know anything about this type.

You shouldn't implement parent_type if you aren't implementing the others.
This was a choice we had to make, between making it easy to implement the ArrayInterface.jl interface vs being conservative.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

Here's a trait example where using the parent type requires indirect information about the wrapper type:

abstract type TraitStyle end

struct FooTrait <: TraitStyle end

struct BarTrait <: TraitStyle end

struct DefaultTrait <: TraitStyle end

abstract type TransformationStyle end

struct UnknownTransformation <: TransformationStyle end

struct FooToBar <: TransformationStyle end


function TraitStyle(::Type{T}) where {T}
    if parenttype(T) <: T
        DefaultTrait()
    else
        transform(TraitStyle(parenttype(T)), TransformationStyle(T))
    end
end

TransformationStyle(::Type{T}) = UnknownTransformation()

transform(::FooTrait, ::FooToBar) = BarTrait()
transform(::FooTrait, ::TransformationStyle) = DefaultTrait()

Sure, you can't blindly access the parent array making any assumption you want, but I don't think that was ever asserted. Blindly using any method is a bad idea.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

Examples from ArrayInterface's tests:

Those tests, as I understand it, would work just as well without the generic definition of:

function contiguous_axis(::Type{T}) where {T}
    if parent_type(T) <: T
        return nothing
    else
        return contiguous_axis(parent_type(T))
    end
end

They rely upon ArrayInterface implementing contiguous_axis{::Type{W}} for a known wrapper W in Union{SubArray, Adjoint, Transpose, PermutedDimsArray} — implementations that are quite straightforward without parent_type. The generic ::Type{T} method with an unknown T means that ArrayInterface is trying to accommodate unknown wrappers, but there's no way it can actually do so meaningfully.

Here's a trait example where using the parent type requires indirect information about the wrapper type:

It looks like that TraitStyle example is slightly better because it's opt-in — a wrapper that simply defines its parent won't get blindly specialized unless they also define TransformationStyle. But at that point, why not just ask the wrapper itself to specialize TraitStyle directly? Then it can decide itself if its "parent" is worth considering in the trait's answer.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

This was a choice we had to make, between making it easy to implement the ArrayInterface.jl interface vs being conservative.

Just for the record, I admit that there are some places we could tight things up in ArrayInterface.jl but the solution isn't to require explicit definitions of every single trait and method there. If you know how indices are transformed to the next layer of an array, then you don't need to know the actual wrapper type. That's the whole basis of traits, appropriately extracting meaningful information even without knowledge of the concrete type.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

I still don't see how parent or parenttype actually help in any of the examples thus far. You still need each wrapper to specialize some method somewhere in addition to parent that actually is doing the important part. The parent itself is a red herring. Implementing parent isn't reducing the number of other methods you need to implement or even taking the place of any of them — it's in addition.

@chriselrod
Copy link
Contributor

Examples from ArrayInterface's tests:

Those tests, as I understand it, would work just as well without the generic definition of:

function contiguous_axis(::Type{T}) where {T}
    if parent_type(T) <: T
        return nothing
    else
        return contiguous_axis(parent_type(T))
    end
end

They rely upon ArrayInterface implementing contiguous_axis{::Type{W}} for a known wrapper W in Union{SubArray, Adjoint, Transpose, PermutedDimsArray} — implementations that are quite straightforward without parent_type. The generic ::Type{T} method with an unknown T means that ArrayInterface is trying to accommodate unknown wrappers, but there's no way it can actually do so meaningfully.

Here's a trait example where using the parent type requires indirect information about the wrapper type:

It looks like that TraitStyle example is slightly better because it's opt-in — a wrapper that simply defines its parent won't get blindly specialized unless they also define TransformationStyle. But at that point, why not just ask the wrapper itself to specialize TraitStyle directly? Then it can decide itself if its "parent" is worth considering in the trait's answer.

You're right, I agree. All it's buying is maybe making it slightly easier to type/DRY w/ respect to where the type parameters are.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

I still don't see how parent or parenttype actually help in any of the examples thus far. You still need each wrapper to specialize some method somewhere in addition to parent that actually is doing the important part. The parent itself is a red herring. Implementing parent isn't reducing the number of other methods you need to implement or even taking the place of any of them — it's in addition.

Let's say we take Adjoint, Transpose, and PermutedDimsArray and give them a unified DimPerm{iperm} <: IndicesTransform type for telling use what happens between the parent array. Now we just need this transformation and the parent for methods like axes, strides, offsets, size, getindex, etc. Yes, the transformations depending on DimPerm still need to be defined, but overall it helps avoid writing all of these methods for all three array types.

But ultimately I'm not suggesting that these traits be implemented or methods changed in this PR.
Nor am I suggesting that this PR should happen because it solves all problems relevant to wrapper arrays.
I'm simply saying that I've found parenttype very useful, it would likely have some utility to others, and it would be complementary to existing methods in Base.

@mbauman
Copy link
Member

mbauman commented Nov 5, 2021

Yeah, I get that. But:

julia> fieldnames(Adjoint)
(:parent,)

julia> fieldnames(Transpose)
(:parent,)

julia> fieldnames(PermutedDimsArray)
(:parent,)

It's really just A.parent — one character shorter, even.

My beef with parent() is that it gives the perception of being generic when it's really not. It doesn't have a sensible definition. Its existence means folks try to do things with it like recursion or things that are inherently broken or construct more complicated systems when a simple trait would do.

But we do have parent. So why not also its type? One concrete reason is that it'll be inconsistent by default for any arrays that have already defined parent.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 5, 2021

My beef with parent() is that it gives the perception of being generic when it's really not. It doesn't have a sensible definition.

Maybe I just don't understand what you mean by "generic". I assumed "generic" meant that parent(x) could be used without knowing what x is. It has a default behavior that by some interpretation could be incorrect, but you could say that about lots of stuff. IndexStyle returns IndexCartesian by default, which could technically be incorrect. Documentation for IndexStyle and parent describe these behaviors. If people want new types to work appropriately they have to write some code.

Its existence means folks try to do things with it like recursion or things that are inherently broken or construct more complicated systems when a simple trait would do.

It's not always appropriate to use but I've already provided examples where it has a real benefit if used correctly. It's not exactly a complicated method, so willing misuse by others is a pretty high bar to overcome.

But we do have parent. So why not also its type? One concrete reason is that it'll be inconsistent by default for any arrays that have already defined parent.

Do you mean it won't return the correct type for arrays that haven't yet defined parenttype but have defined parent? Wouldn't we just need to put a version warning in the doc string? It's not like this would break any current code.

Edit:

  • When is it an issue to implement a new method that is related to previous behavior? ComposedFunction is a different type than what use to return, but it was acceptable to add into base.

@mbauman
Copy link
Member

mbauman commented Nov 9, 2021

I assumed "generic" meant that parent(x) could be used without knowing what x is.

Yes, that's precisely my working definition as well — and my argument throughout here is that you cannot do this with the parent function. The default fallback implementation isn't so much the trouble — it's that the definition of the function itself doesn't say what it means to be a "parent array" or the kinds of transformations that might get applied along the way. And thus you don't know what's valid to be done with the parent once you get it. This is significantly different from what IndexStyle does.

Now, yes, you can define secondary methods that say "alright, now here's how you can interpret that parent," but my argument is that it's a roundabout sort of solution that would be better as a single direct question you ask of the wrapper that is completely and fully defined in a generic fashion.

Do you mean it won't return the correct type for arrays that haven't yet defined parenttype but have defined parent? Wouldn't we just need to put a version warning in the doc string? It's not like this would break any current code.

Yes, that's precisely what I mean. It means that typeof(parent(A)) !== parenttype(A) by default for all the interesting cases unless folks learn about the need to implement it. It's not necessarily a showstopper, but it leaves a bad taste in my mouth in addition to all my other reservations.

In terms of what we're allowed to do within the bounds of semantic versioning, adding a new function is always legal, but that doesn't mean it's always a good idea. Changing the output of an existing function (e.g., changing the return type of ) is a completely different situation from adding a new export.

@Tokazama
Copy link
Contributor Author

Now, yes, you can define secondary methods that say "alright, now here's how you can interpret that parent," but my argument is that it's a roundabout sort of solution that would be better as a single direct question you ask of the wrapper that is completely and fully defined in a generic fashion.

Traits depending on other traits isn't exactly an unheard of idea. For example, this is a lot like trait contracts in BinaryTraits.jl. I guess you could argue that entire approach is bad and people should stop pursuing it, but there are also plenty of registered packages that define parenttype or parent_type independent of the method in ArrayInterface. I haven't audited each one, but even if they just use it as a convenient way of accessing the that type in a parametric type, it appears to be useful to plenty of people.

I'm just trying to contribute to the Julia community. I know I don't exactly have a whole lot of weight behind my name here, so if you don't want this method out of principle that's fine. I'm not going to be a thorn in anyone's side here.

In terms of what we're allowed to do within the bounds of semantic versioning, adding a new function is always legal, but that doesn't mean it's always a good idea. Changing the output of an existing function (e.g., changing the return type of ) is a completely different situation from adding a new export.

I understand their different, but my point is that changing a return type is technically a breaking change. That seems a lot more disruptive than what I've proposed.

@mbauman
Copy link
Member

mbauman commented Nov 10, 2021

Yes, I'm really sorry to be so negative about your first pull request here — and I must say I'm really thrilled to see the things that are happening in ArrayInterface.jl. It's addressing a huge pain point that I wish I would've had more time to help with. Your name does carry weight, but it doesn't really matter if you were Jeff himself or someone completely new to the ecosystem; my comments would be the same.

My negativity here is all about parent itself and predates this PR, born out of my own frustration in trying to use it for our rudimentary aliasing checks... which is a place where it really feels like it should at least be able to work. But it doesn't. My comments here aren't the last word nor are they about trying to prevent this from happening at all costs. Rather, my goal is to warn you about this tarpit as someone who's gotten stuck in it myself... and hopefully convince you that charging full-speed into it isn't a great idea.

@Tokazama
Copy link
Contributor Author

Sorry for the slow response, but I think most of what needs to be said here has been.

hopefully convince you that charging full-speed into it isn't a great idea.

We've been able to do most of what we need to do with the current array interface so far. Therefore, I think the utility of most new methods/traits will be low impact for most users. In my efforts to defend the addition of parenttype, I may have been misleading as to how prevalent/important I think its usage should be. I agree that 99% of the time it doesn't make sense to have a trait/method depend on a parenttype paired with another trait.

If concerns are specifically due to how it is used in ArrayInterface, then I'd like to make it clear that I'm aware of it's poor usage in several places and have been making steps towards fixing it. This PR is a big effort to resolve this issue (and others) by pairing parent with the concept of "layouts". So when I said it is "a critical component for a lot of methods in ArrayInterface.jl" it should also be known that isn't a claim that all of its usage therein is a clean template for others to copy or that it should always be used.

I'm not aware of all the other ways people have used parent previously, so I'm not really able to address those. I welcome any suggestions you may have in clarifying documentation to prevent misuse here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants