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

specific getindex for single vs. slice #8

Closed
wants to merge 2 commits into from
Closed

Conversation

rkurchin
Copy link
Collaborator

Thought it might make sense to explicitly include both of these options, per our discussion last Friday. I think we should also split out the sample implementations and rename the current implementation_simple to implementation_simple_aos or something like that and then add an analogous SoA version, which would need to explicitly dispatch these separately.

Not sure this actually makes sense to merge as-is though, since currently I think it would also break the existing implementation_simple, and we may want to have things set up for that case to be able to "just work." If so, I think we'd just need to define the slice dispatch to be the standard Julia behavior.

So yeah, this is more to start a conversation I suppose? But getting it to a non-draft state should be pretty simple once we decide what we want the behavior to be.

Copy link
Collaborator

@Gregstrq Gregstrq left a comment

Choose a reason for hiding this comment

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

Even if we focus our attention on a single index, there is room for variation. For example, for lattices, the index might be of type CartesianIndex; if we want to add some kind of grouping, we can have a composite index (i, ig), where ig is the group index, while i is an index inside the group.
So, may be we should not specify the type of single index:

Base.getindex(::AbstractSystem{AT}, I)::AT = error("Implement me").

There is another problem: we use AT<:AbstractParticle as the type parameter for AbstractSystem, which kind of favours array of structs implementation. May be we should instead use ET<:AbstractElement as the type parameter for AbstractSystem and define AbstractParticle as

abstract type AbstractParticle{ET<:AbstractElement} end

@rkurchin
Copy link
Collaborator Author

Fair point. So what's the cleanest way to distinguish between the two return types, i.e. when a new AbstractSystem should be returned vs. a single particle?

@Gregstrq
Copy link
Collaborator

Looks like the ranges are normally subtypes of AbstractArray, with the exception of : (full slice) which is of type Colon.
It seems that we can simply use:

Base.getindex(::AbstractSystem{AT}, I)::AT = error("Implement me")
Base.getindex(::AbstractSystem, ::T)::AbstractSystem where {T<:Union{AbstractArray, Colon}}  = error("Implement me")

@rkurchin
Copy link
Collaborator Author

Should we also make I there a union type to explicitly accommodate integers and single Cartesian indices? Or are there other types of indices beyond that we'd want/need to support? I suppose we could just clarify what the two methods are with a comment...

@Gregstrq
Copy link
Collaborator

I think saying that this version of getindex is for single index is enough.

Signed-off-by: Rachel Kurchin <[email protected]>
@mfherbst
Copy link
Member

I wonder if not specifying the type of the I for the single-argument getindex is too general. I mean the lattice use case of cartesian indexing / group indexing is not that common and nothing prevents you from adding that in a more specialised struct. The most general thing that is still useful is just a single homogeneous integer index (like the linear index interface arrays of any dimension have in julia). Behind the scenes that might sill be converted to something more elaborate.

@rkurchin
Copy link
Collaborator Author

Yeah, that was sort of what I was thinking as well... 🤔

@Gregstrq
Copy link
Collaborator

The most general thing that is still useful is just a single homogeneous integer index (like the linear index interface arrays of any dimension have in julia). Behind the scenes that might sill be converted to something more elaborate.

It is true, that the linear index can be converted to other types of indices, but it may take time. For example, N-dimensional arrays in base julia support Cartesian Indices by default. As a result, you can directly index with Cartesian Indices, but if you index with a linear index, an explicit index conversion is required.

@rkurchin rkurchin mentioned this pull request Nov 16, 2021
@rkurchin
Copy link
Collaborator Author

Linear indexing is addressed in #23, closing this for now and opening an issue to potentially support Cartesian indexing in the future.

@rkurchin rkurchin closed this Nov 17, 2021
@mfherbst mfherbst deleted the rk/slicing branch January 28, 2022 19:44
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