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

RFC: Add Vector and Matrix constructors #11154

Merged
merged 1 commit into from
May 8, 2015

Conversation

garrison
Copy link
Member

@garrison garrison commented May 6, 2015

One of my very earliest stumblings with julia was trying to create empty vectors. The syntax Int[] was non-obvious to me, and I kept trying to do things like Vector{Int}(). #3214 is a step in the right direction, as it allows Vector{T}(0). But is there any reason not to allow Vector{T}()? It would be in line with Dict() (or Dict{K,V}()), which creates an empty dictionary. (Not to mention C++'s auto v = std::vector<int>();, python's x = list(), etc., which are in the same style and may form a mental model for people coming from other languages.)

On the other hand I don't think Matrix{T}() makes sense, as matrices are not meant to be easily resized in the same way vectors can be, so I have not implemented it.

@jakebolewski
Copy link
Member

I agree that this is missing. could you add a simple test?

@quinnj
Copy link
Member

quinnj commented May 6, 2015

+1, this is something I've wanted for a while. I wonder if we deprecate Int[] as well as it's come up a few other times as being "in the way" syntax for other things. We could also consider doing []Int, a la Go, to be shorthand for this.

@GunnarFarneback
Copy link
Contributor

Empty matrices make perfect sense. Even if you can't resize them you can replace them while keeping type stability. It could be argued that it's better to have to be explicit about constructing a 0x0 matrix rather than some other shape of empty matrix, however.

@StefanKarpinski
Copy link
Member

Vector{Int}(x,y,z) might be a good alternative syntax to Int[x,y,z] – which looks nice but is kind of a hack since it's actually an indexing operation into a type object. You could potentially write a typed matrix literal as something like Matrix{Float64}([1,2,3],[4,5,6],[7,8,9]), but it's a bit awkward.

@jiahao
Copy link
Member

jiahao commented May 6, 2015

For comparison, OrderedSet in DataStructures.jl used to supported a constructor of the form

OrderedSet(1, 2, 3)

but this was deprecated in favor of

OrderedSet([1, 2, 3])

@garrison
Copy link
Member Author

garrison commented May 6, 2015

Speaking of tests, here is an interesting surprise (with or without having applied this patch): (reported separately in #11180)

julia> Array{Int}()
0-dimensional Array{Int64,0}:
139928966006256

@garrison
Copy link
Member Author

garrison commented May 6, 2015

@jiahao, same with Set itself in julia (Set(1,2,3) works in 0.3 but not 0.4).

@garrison garrison force-pushed the vector-default-constructor branch 3 times, most recently from 5a96bd4 to 7bd605a Compare May 6, 2015 17:02
@garrison
Copy link
Member Author

garrison commented May 6, 2015

OK, I added some tests.

I also made Vector() return an empty Vector{Any}, just as Dict() returns returns an empty Dict{Any,Any}.

Aside from the question of whether Matrix{T}() should make an empty matrix, there is the question of whether Matrix(n, m) should create an n by m Matrix{Any}. I feel like it is rare that a user will actually want Matrix{Any} so have left it out, but it is something to consider and could be added easily.

@@ -15,7 +15,8 @@ typealias StridedVector{T,A<:DenseArray,I<:Tuple{Vararg{RangeIndex}}} Union(Den
typealias StridedMatrix{T,A<:DenseArray,I<:Tuple{Vararg{RangeIndex}}} Union(DenseArray{T,2}, SubArray{T,2,A,I})
typealias StridedVecOrMat{T} Union(StridedVector{T}, StridedMatrix{T})

call{T}(::Type{Vector{T}}, m::Integer) = Array{T}(m)
call{T}(::Type{Vector{T}}, m::Integer=0) = Array{T}(m)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not call{T}(::Type{Vector{T}}) = Array{T}(0)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, in an earlier revision I added precisely this line instead of doing the current replacement. Why do you prefer it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess your way would be preferable if we were to want to generalize to Matrix as well, because then we would not want a single-argument constructor to work. Is there any other reason to prefer the method you suggest?

@garrison garrison force-pushed the vector-default-constructor branch from 7bd605a to d132337 Compare May 8, 2015 00:35
@garrison garrison changed the title RFC: Add Vector{T}() constructor RFC: Add Vector and Matrix constructors May 8, 2015
@garrison
Copy link
Member Author

garrison commented May 8, 2015

I went ahead and implemented each constructor for Matrix as well. After considering it more, I saw no good reason not to have them.

@GlenHertz
Copy link
Contributor

+1. The Int[1,2,3] syntax (and siblings) got me hung up too initially, especially for Dicts. It feels like indexing instead of creation. Vector{Int}(1,2,3) and Vector{Int}() would be a big improvement.

jakebolewski added a commit that referenced this pull request May 8, 2015
RFC: Add Vector and Matrix constructors
@jakebolewski jakebolewski merged commit f2e61a1 into JuliaLang:master May 8, 2015
@GlenHertz
Copy link
Contributor

I just realized that the constructor is Vector{T}(size) and not Vector{T}(element1, el2, el3, ...). I think the more common case is you know the values during construction and you can use sizehint! to set the size (optional). So with this patch if you want to create a vector of [1,2,3,4] would you do:

v = Vector{Int}(4)
v[1] = 1
v[2] = 2
v[3] = 3
v[4] = 4
v[5] = 5  # this will grow

(Hopefully I'm missing a better way to do it)

I think it would be better to have:

x = Vector{Int}(1,2,3,4)
y = sizehint!(Vector{Int}(), 500)
z = Matrix{Float64}([1,2,3],[4,5,6],[7,8,9])

Would it help if we had a Size type (like we now have a type for Indices) to make the interface less confusing, (x = Vector{Int}(Size(500)), z = Matrix{Float64}(Size(3,3),1,2,3,4,5,6,7,8,9)).

@jiahao
Copy link
Member

jiahao commented May 8, 2015

I'm really not convinced that the default empty Matrix() constructor makes any sense. Unlike empty vectors, empty matrices are not mutable iterable containers. You can't push!, append! or pop! anything from a matrix. In this respect, Vector and Matrix really are different. (Ref: #4774)

@garrison
Copy link
Member Author

garrison commented May 8, 2015

@GlenHertz I think the best existing way to create the vector [1,2,3,4] is to explicitly type [1,2,3,4]. The code you post will error on the line v[5] = 5 with a BoundsError. Of the lines you suggest, y already works, but x and z are in conflict with current syntax. I'd suggest opening another issue to discuss this proposed syntax change, which is in line with what @StefanKarpinski suggested above.

@garrison
Copy link
Member Author

garrison commented May 8, 2015

@jiahao I was ultimately swayed by @GunnarFarneback's argument that at times you want to explicitly create an empty matrix for type stability. I've had a need for such a thing in my own code in the past. EDIT: of course, I'm open to further discussion on this.

@jiahao
Copy link
Member

jiahao commented May 8, 2015

I still don't understand the utility of the Matrix() constructor. Code that can benefit from type stability won't be using Matrix{Any} , which is the type constructed from Matrix(...). The danger is that people might write

f() = randbool() ? Matrix() : randn(10,10)

thinking it is type stable, but it is not, because of parametric invariance. The return type can only be analyzed statically to yield at best Union(Matrix{Any}, Matrix{Float64}), which is not concrete. You'd still have to write something like

Array(Float64,0,0)

instead of Matrix() for type stability.

@garrison
Copy link
Member Author

garrison commented May 8, 2015

Array(Float64,0,0)

Or Matrix{Float64}().

Note too that the argument you make could also be made for Vector().

@jiahao
Copy link
Member

jiahao commented May 8, 2015

Yes, but my earlier point is that one can want to have Vector{Any} as a mutable list, whereas Matrix{Any}, the default output type of Matrix(), can't be used that way. I can't think of any serious use for Matrix{Any}.

@garrison garrison deleted the vector-default-constructor branch May 9, 2015 20:00
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

Successfully merging this pull request may close these issues.

7 participants