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

[SparseArrayDOKs] [ENHANCEMENT] resize! and @maybe_grow #1433

Closed
mtfishman opened this issue May 12, 2024 · 4 comments · Fixed by #1434
Closed

[SparseArrayDOKs] [ENHANCEMENT] resize! and @maybe_grow #1433

mtfishman opened this issue May 12, 2024 · 4 comments · Fixed by #1434
Labels
enhancement New feature or request NDTensors Requires changes to the NDTensors.jl library. SparseArrayDOKs

Comments

@mtfishman
Copy link
Member

mtfishman commented May 12, 2024

It would be helpful to make the size of SparseArrayDOK a mutable field (and maybe define an IsResizable trait).

Then we can have an API for setting the size of a SparseArrayDOK in-place with:

function resize!(a::SparseArrayDOK{<:Any,N}, new_size::NTuple{N,Integer}) where {N}
  a.dims = new_size
  return a
end

and additionally functionality for setting an element and allowing the array to grow as needed:

function setindex_maybe_grow!(a::SparseArrayDOK{<:Any,N}, value, index::NTuple{N,Int}...) where {N}
  if any(index .> size(a))
    resize!(a, max.(index, size(a)))
  end
  a[index...] = value
  return a
end

which could have an external API based around annotating calls to setindex! with a macro @maybe_grow:

a = SparseArrayDOK{Float64}(3, 3)
a[4, 4] = 44.0 # Calls `setindex!, errors as out-of-bounds.
@maybe_grow a[4, 4] = 44.0 # Calls `setindex_maybe_grow!`, resizes to `(4, 4)` and sets the element.

@emstoudenmire

@mtfishman mtfishman added enhancement New feature or request ITensors Issues or pull requests related to the `ITensors` package. NDTensors Requires changes to the NDTensors.jl library. SparseArrayDOKs and removed ITensors Issues or pull requests related to the `ITensors` package. labels May 12, 2024
@emstoudenmire
Copy link
Collaborator

I like this proposal & will try to work on it if I can figure out the macro part. Other name suggestions could be @auto_resize or @allow_resize.

@mtfishman
Copy link
Member Author

I like the terminology grow instead of resize since it makes it clearer that it will only (potentially) grow the array size, and never shrink it.

@emstoudenmire
Copy link
Collaborator

Some thought may be needed about constructors also, given this system. While the following works:

m = SparseArrayDOK{Float64,2}((0, 0))

the default constructor errors:

m = SparseArrayDOK{Float64,2}() # gives an error

It's not clear to me whether it's best to require users to give an initial size if they are going to use the grow feature, and if they would give (0,0) then it might be good to let them default initialize, unless that causes other problems.

@mtfishman
Copy link
Member Author

mtfishman commented May 12, 2024

I think we should just follow the convention followed by other Julia AbstractArray constructors, which is that if no size is specified it constructs a zero-dimensional SparseArrayDOK:

julia> Array{Float64}(undef)
0-dimensional Array{Float64, 0}:
2.225328024e-314

julia> zeros()
0-dimensional Array{Float64, 0}:
0.0

SparseArrayDOK{Float64,2}() therefore shouldn't be allowed:

julia> Array{Float64,2}(undef)
ERROR: MethodError: no method matching Matrix{Float64}(::UndefInitializer)

Closest candidates are:
  Matrix{T}(::UndefInitializer, ::Tuple{Int64, Int64}) where T
   @ Core boot.jl:487
  Matrix{T}(::UndefInitializer, ::Int64, ::Int64) where T
   @ Core boot.jl:479
  Array{T, N}(::UndefInitializer, ::Tuple{Vararg{Int64, N}}) where {T, N}
   @ Core boot.jl:489
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

and users should be required to write SparseArrayDOK{Float64,2}(0, 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request NDTensors Requires changes to the NDTensors.jl library. SparseArrayDOKs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants