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

WIP/RFC: allow resize! to initialize new elements #16769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rfourquet
Copy link
Member

This makes resize!(A, n, default) initialize new elements with a default value in case of (positive) growth. A more general version resize(filler, A, n) is also introduced which initializes A[i] with filler(i). This can be useful in particular when the default values must be distinct objects (e.g. resize!(_->Int[], Vector{Vector{Int}}(), 2)).

For reference, the corresponding resize function in C++ accepts such a default value (and also rust apparently).

I implemented this by adding a method to fill!, but I'm not sure it's the right solution: fill!(filler, A::AbstractArray, start::Integer, stop::Integer).
If this becomes public API, it would then makes sense to have default values for start/stop: fill!(filler, A::AbstractArray), but then there could be ambiguity with fill!(A::AbstractArray, default) if default is of type AbstractArray.
I can simply rename this method to _fill! to postpone the discussion of an extended fill! API.

If the idea receives support, I will write docs/tests and a similar version for BitArray{1} should also probably exist.

@tkelman tkelman added needs tests Unit tests are required for this change needs docs Documentation for this change is required labels Jun 5, 2016
@nalimilan
Copy link
Member

Makes sense. Though that signature would conflict with #16737, so if we want both we need to find an alternative somewhere.

@rfourquet
Copy link
Member Author

Maybe the atend of the other issue could be a keyword argument?

@nalimilan
Copy link
Member

Probably.

@timholy
Copy link
Sponsor Member

timholy commented Jun 5, 2016

👍. The version without start/stop will also be important for arrays that don't have efficient linear indexing. To avoid the ambiguity, perhaps you simply have to declare filler::Function in this case. (fill!(filler, A::AbstractArray) would be ambiguous with fill!(A::AbstractArray, x) no matter what eltype A has.)

@nalimilan
Copy link
Member

Is filler::Function really a good idea? Why not have a copy=false keyword argument which would enable copying the provided element before assigning it? resize!(Int[], x, 2, copy=true) looks nicer and more explicit than resize!(_->Int[], x, 2).

@rfourquet
Copy link
Member Author

[Sorry for the long post, cf. conclusion at the end]
I'm not sure about resize!(A, 2, Int[], copy=true), because it's not immediately obvious what copy means in the context of a resizing operation. And I like the more general function argument (idiomatic Julia?), which allows for example resize!(_->rand(Int), A, 2). But I admit I'm not sold on the fact that this function should take the index as first argument (I modeled it after ntuple).

Concerning "arrays that don't have efficient linear indexing": would it make sense to have an advance function (like in C++) which would efficiently (when possible) advance the state of eachindex(itr) ? So that the two loops below would be equivalent, but the second possibly faster:

for i in start:stop
    A[i] = ...
end

EI = eachindex(A)
for i in advance(EI, start):advance(EI, stop)
    A[i] = ...
end

Again, I'm not sure about the fact that fill! should receive a one-argument function. I've prepared recently a branch which introduces a function generate/generate! (named after the C++ equivalent, but possibly confusing in Julia given the Generator type), which fills an array by calling repeatedly a zero-argument function. This could be clearer than modifying fill!.

In conclusion, I propose to leave fill! unchanged for now, and to decide for resize! one of the 3 options:

  1. resize!(A, 2, Int[], copy=true)
  2. resize!(_->Int[], A, 2)
  3. resize!(()->Int[], A, 2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs docs Documentation for this change is required needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants