add deleteat! method

[vlepori]

As discussed in #55. I kept the deleteat!(A,(),i) API.

The following are supported

a = ElasticArray(rand(7, 7, 7))
deleteat!(a, (), (), ())
deleteat!(a, (), (), 6)
deleteat!(a, (), (), [1, 5])
deleteat!(a, (), (), 3:4)

And this errors

deleteat!(a, 1:2, (), 1:2)
# ERROR: ArgumentError: Can only delete elements in the last dimension of A

[oschulz]

@vlepori looking at this again, maybe the primary way to do it should be deleteat!(A, ((), i)), but we should support deleteat!(A, (), i) as well. It matches how handle resize! in

ElasticArrays.jl/src/elasticarray.jl

Line 108 in 7e0b6f8

@inline function Base.resize!(A::ElasticArray{T,N}, dims::NTuple{N,Integer}) where {T,N}

.

Could you change idxs::Vararg{...} to idxs::NTuple{...} in the primary method and then add a convenience method with idxs::Vararg{...} like in

ElasticArrays.jl/src/elasticarray.jl

Line 106 in 7e0b6f8

@inline Base.resize!(A::ElasticArray{T,N}, dims::Vararg{Integer,N}) where {T,N} = resize!(A, dims)

?

[oschulz]

Oh, and could you add a test or two?

[vlepori]

Good point, both APIs are supported now. I also added a couple of tests.

[oschulz]

Why wrap this in a function?

[vlepori]

Just consistency, since other functions are tested with a functionname_test(). But we can also call test_E() directly

[oschulz]

But we can also call test_E() directly

Let's to that, it's simpler. I mainly wrapped tests if there are called several times with different arrays sizes or so.

[vlepori]

Yeah that makes sense, I simplified it.

[oschulz]

Can you add an @inferred here?

[oschulz]

An @inferred might be nice here as well.

[oschulz]

Ok, let's run Ci on it then. :-)

[codecov]

Codecov Report

Merging #56 (34d0809) into main (03bfd5d) will decrease coverage by 0.25%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
- Coverage   84.74%   84.49%   -0.25%     
==========================================
  Files           2        3       +1     
  Lines         118      129      +11     
==========================================
+ Hits          100      109       +9     
- Misses         18       20       +2     

Impacted Files	Coverage Δ
src/elasticarray.jl	84.00% <80.00%> (-0.35%)	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[oschulz]

Hm, looks like there trouble on v1.0 .

[vlepori]

At least one problem here is that the method first(v,i) was introduced only in Julia 1.6. But since idxs is a Tuple I think we can safely change first(idxs, length(idxs) - 1) to idxs[1:end-1] and not worry about non-standard indexing? I can update the PR later today.

[oschulz]

But since idxs is a Tuple I think we can safely change first(idxs, length(idxs) - 1) to idxs[1:end-1] and not worry about non-standard indexing?

Yes, I think that should be Ok.

[oschulz]

Hey @vlepori I'm so sorry this got stalled, I completely forgot to click "run CI" ...

[oschulz]

@vlepori could you take a look at the failing tests?

[vlepori]

@oschulz yes sorry, I'll try to get a working installation of Julia 1.0 and look into this soon !

[oschulz]

Thanks @vlepori , no worries!

[jeremiahpslewis]

Would love to see this added, could be great to use it to implement pop!

[oschulz]

@vlepori want to get back on this?

[vlepori]

Sure! Got sidetracked and forgot about this but if there is interest I will try to get it done!

[vlepori]

So I went down the rabbit hole behind this failing test on julia 1.0

ElasticArrays.jl/test/elasticarray.jl

Line 310 in 34d0809

@test selectdim(E, n, 2:s) == @inferred deleteat!(b, ntuple(i -> i == n ? 1 : (), n))

It seems to have nothing to do with ElasticArrays but seems related to a weird side effect of the @test macro in older versions of Julia. I can reproduce it in Julia 1.0.3 like so

using Test

function fun()
    d = 2
    @test true # works as aspected when commenting out this line
    ntuple(i -> i == d ? true : false, 2)
end
fun() # should return (false,true) but gives (true,true)

I don't quite understand further (?) but the bug has been fixed in more recent versions, so maybe just skip the test ?

[oschulz]

Hm, now that v1.10 is set to become the new LTS, maybe it's time to drop v1.0 support?

[vlepori]

Not sure if it's necessary for just this one test, but I'd be surprised if many people still use 1.0. For comparison DifferentialEquations stopped supporting it 4 years ago and now requires julia 1.9

[oschulz]

Looks like among the package that depend on ElasticArrays, the packages EfficientGlobalOptimization, StanSample and SphericalHarmonics still (claim to ) support Julia v1.0.

So I suggest we do indeed skip the test, but keep v1.0 compatibility for now.