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

deleteat! by CartesianIndex #41170

Closed
wangl-cc opened this issue Jun 10, 2021 · 4 comments
Closed

deleteat! by CartesianIndex #41170

wangl-cc opened this issue Jun 10, 2021 · 4 comments
Labels
arrays [a, r, r, a, y, s]

Comments

@wangl-cc
Copy link
Contributor

I try to remove the item of vector at a CartesianIndex, like

julia> deleteat!([1, 2], CartesianIndex(1))
ERROR: iteration is deliberately unsupported for CartesianIndex. Use `I` rather than `I...`, or use `Tuple(I)...`

It seems that a CartesianIndex is treated as an iterator of indices instead of an index, and there is not a method like deleteat!(a::Vector, ind:: CartesianIndex).
Is it possible to add a method deleteat!(a::Vector, ind:: CartesianIndex)?

@johnnychen94
Copy link
Member

This might not be as useful as you thought it be. Although there're some intersections, deletat! is a method for list-like objects while CartesianIndex is mostly for multi-dimensional arrays. Only specializing deletat!(a::Vector, ind::CartesianIndex{1}) doesn't sound a good change to me.

@johnnychen94 johnnychen94 added the arrays [a, r, r, a, y, s] label Jun 10, 2021
@wangl-cc
Copy link
Contributor Author

@johnnychen94 Thanks for your reply. Specializing deleteat!(a::Vector, ind:: CartesianIndex) doesn't sound good, but the error message is quite confusing. Especially, delete by a vector of CartesianIndex:

julia> deleteat!([1, 2], [CartesianIndex(1), CartesianIndex(2)])
ERROR: MethodError: no method matching +(::CartesianIndex{1}, ::Int64)

I think that deleteat! convert the CartesianIndex to integer index or just throw an ArgumentError may be better.

@johnnychen94
Copy link
Member

By saying deleteat! is a method for list-like objects, I was saying that this is expected to only work on linear indices. So my two cents is, if we don't plan to support deleteat! for multi-dimensional array a::Array, we don't need to support inds::CartesianIndex. Clearly that we don't plan to support it because the only way to do it is vectorizing it, e.g., vec(a).

A similar case to this is JuliaArrays/OffsetArrays.jl#137.

I think that deleteat! convert the CartesianIndex to integer index or just throw an ArgumentError may be better.

Both of these are doable, one could convert a CartesianIndex into a LinearIndex, e.g., by calling deleteat!(x, LinearIndices(x)[inds])) or even do some lazy conversions. But as I just said, I don't think we need to add this specialization; it might be convenient or even more consistent at first sight, but IMO it eventually adds confusion because CartesianIndex is not designed for the vector method.

It's just my two cents and people may disagree, though.

@wangl-cc
Copy link
Contributor Author

@johnnychen94 Thanks for your patience. Maybe you're right.

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]
Projects
None yet
Development

No branches or pull requests

2 participants