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 method for deleteat! #55

Open
vlepori opened this issue Mar 31, 2023 · 5 comments
Open

Defining a method for deleteat! #55

vlepori opened this issue Mar 31, 2023 · 5 comments

Comments

@vlepori
Copy link

vlepori commented Mar 31, 2023

It would be nice to have a convenience function to remove any column/slice from the last dimension of an ElasticArray. An implementation that currently only works for an ElasticMatrix could look like this.

function Base.deleteat!(A::ElasticArray{T,N}, at::Union{UnitRange{I}, I}) where {T,N,I <: Integer}
    idx = LinearIndices(A)[:, at]
    deleteat!(A.data, idx)
    return A
end

u = ElasticArray{Float64}([0.0 1.0 2.0; 0.0 1.0 2.0; 0.0 1.0 2.0])

#3×3 ElasticMatrix{Float64, Vector{Float64}}:
 #0.0  1.0  2.0
 #0.0  1.0  2.0
 #0.0  1.0  2.0

deleteat!(u, 2:3)

#3×1 ElasticMatrix{Float64, Vector{Float64}}:
#0.0
#0.0
#0.0

Do you think this would make sense? Is there an easy equivalent way to do the same already?

@oschulz
Copy link
Collaborator

oschulz commented Mar 31, 2023

I'm not sure - the performance (copy! combined with resize! may be faster than the one above) would not be higher than for a regular array, since a lot of memory might have to be move (except when deleting one of the last slices).

Do you have a specific use case in mind, @vlepori ?

We'd also have to think about the API. The assumption that deleteat!(A, i) automatically addresses the (whole) last dimension may run a bit counter to the standard interpretation of deleteat!.

@vlepori
Copy link
Author

vlepori commented Mar 31, 2023

Good points - I must say performance wasn't my main concern here. Rather, code readability: I find that being able to deleteat! certain positions would be clearer and less error-prone than resize!ing and copying values around. My specific use case is that I use an m x n ElasticMatrix as the state variable in an ODE, and I shrink n (using DifferentialEquations 's callbacks) as the system changes dimensionality during integration.

True regarding the API, maybe deleteat!(u,(:,2:3)) would be clearer and more consistent with resize!

@oschulz
Copy link
Collaborator

oschulz commented Mar 31, 2023

My specific use case is that I use [...]

Right, makes sense. So let's do it, I'd say. Implementation wise, I'd suggest to use A[...] = A[...] and resize!, it'll be fast and allocation-free.

True regarding the API, maybe deleteat!(u,(:,2:3))

Hm but that could be read as "delete everything in the first dim and 2:3 in the second ...

How about deleteat!(u, (), 2:3) ? deleteat!(rand(10), ()) seems valid and leaves the array unchanged.

Would you like to do a PR?

@vlepori
Copy link
Author

vlepori commented Mar 31, 2023

Personally, I read it as the subsetting indices u[:,2:3], i.e. we want to delete all the elements whose index i is any, and j is 2:3. But perhaps (u, (), 2:3) is indeed less ambiguous. Either way, deleting dims other than the last is anyway against the main package idea, hopefully that is not lost on users..
And yes, happy to try my hand at a PR using resize! for the implementation.

@oschulz
Copy link
Collaborator

oschulz commented Mar 31, 2023

And yes, happy to try my hand at a PR using resize! for the implementation.

Great, thanks!

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