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

defining a custom getindex on a AbstractBasicDimArray is tricky #890

Open
tiemvanderdeure opened this issue Jan 2, 2025 · 4 comments
Open

Comments

@tiemvanderdeure
Copy link
Collaborator

I am implementing the AbstractBasicDimArray interface on a bunch of objects in SDM.jl, and while lots of things work like a charm, getindex and siblings are really tricky to disambiguate.

The way I am doing this currently is that these objects have one field that is always a DimArray and does most of the work, and one or more fields with some extra info that shouldn't really be touched by something like getindex.

Maybe this is the price you pay for all the really great features of this package, but we could look into ways of making this less cumbersome. The low-hanging fruit would be to include an example in the docs (like for AbstractDimStack, if we end up making that a AbstractBasicDimArray), but maybe it's even possible to define a macro that does some of this work?

@rafaqz
Copy link
Owner

rafaqz commented Jan 2, 2025

Yes it's complicated, it's hard enough handling ambiguity internally. But why do you need to add getindex methods, and which ones?

@tiemvanderdeure
Copy link
Collaborator Author

My idea here was to have an object that contains an array of MLJ machines but has SDM machines as an eltype, basically like so:

using DimensionalData
const DD = DimensionalData

struct MyEltype
    x
end

struct MyDimArray{N, D} <: DD.AbstractBasicDimArray{MyEltype, N, D}
    data::DimArray{<:Any, N, D}
end

DD.dims(d::MyDimArray) = DD.dims(d.data)

function Base.getindex(d::MyDimArray, I...; kw...)
    obj = Base.getindex(d.data, I...; kw...)
    if obj isa DD.AbstractDimArray
        MyDimArray(obj)
    else
        MyEltype(obj)
    end
end

Now this works great, except it generates a whole lot of method ambiguities. E.g.

myda = MyDimArray(rand(X(1:10), Y(1:10)))
myda[DimIndices(myda)]
ERROR: MethodError: getindex(::MyDimArray{2, Tuple{X{…}, Y{…}}}, ::DimIndices{Tuple{X{…}, Y{…}}, 2, Tuple{X{…}, Y{…}}}) is ambiguous.

Candidates:
  getindex(d::MyDimArray, I...; kw...)
    @ Main ~/juliadev/SpeciesDistributionModels/test/runtests.jl:105
  getindex(A::DimensionalData.AbstractBasicDimArray, i::DimIndices)
    @ DimensionalData ~/.julia/packages/DimensionalData/ejVFv/src/array/indexing.jl:102
  getindex(A::DimensionalData.AbstractBasicDimArray, d1::Union{Tuple{DimensionalData.Dimensions.Dimension, Vararg{DimensionalData.Dimensions.Dimension}}, AbstractArray{<:DimensionalData.Dimensions.Dimension}, AbstractArray{<:Tuple{DimensionalData.Dimensions.Dimension, Vararg{DimensionalData.Dimensions.Dimension}}}, DimIndices, DimSelectors, DimensionalData.Dimensions.Dimension}; kw...)
    @ DimensionalData ~/.julia/packages/DimensionalData/ejVFv/src/array/indexing.jl:83

Possible fix, define
  getindex(::MyDimArray{N, D} where {N, D<:Tuple}, ::DimIndices)

AFAIK I would have to work through all of those to get this working? Or am I just doing this all wrong?

@rafaqz
Copy link
Owner

rafaqz commented Jan 3, 2025

function Base.getindex(d::MyDimArray, I::Int...)

This should be safe

@rafaqz
Copy link
Owner

rafaqz commented Jan 3, 2025

function Base.getindex(d::MyDimArray, I:;Int...)

This should be safe, but also that looks like an AbstractDimArray not AbstractBasicDimArray. In which case just define parent and it will work

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

No branches or pull requests

2 participants