JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.68k stars 5.48k forks source link

decide if `[1 2 3]` should be a matrix or row vector #23018

Closed StefanKarpinski closed 6 years ago

StefanKarpinski commented 7 years ago

There was some discussion of this now that we have row vectors.

ararslan commented 7 years ago

Would this refer to the space-delimited array literal only or also to hcat on scalars? Seems like the two should be consistent.

dlfivefifty commented 7 years ago

+1 for RowVector

DrKrar commented 7 years ago

+1 for RowVector

andreasnoack commented 7 years ago

+1 for Matrix because then we have simple ways to construct a Matrix ([1 2 3]) as well as a RowVector ([1,2,3]').

dlfivefifty commented 7 years ago

Won't [1 2 3;] still create a matrix either way?

andreasnoack commented 7 years ago

Good question. I don't know. That might be part of the decision here.

JeffBezanson commented 7 years ago

Yes, the parser distinguishes [1 2 3] and [1 2 3;], which could be useful for this case, but which I'm also not a fan of. It means there are three syntax forms ([1,2], [1 2], and [1 2; ...]) for each kind of brackets, which will start to be annoying if we add more brackets. Currently {1 2} and {1 2;} parse the same.

perrutquist commented 7 years ago

It would be an interesting experiment to implement this and see how much code it breaks in published packages. My guess is that most code would keep running, and it will turn out to be very rare that people actually wanted a 1-by-n matrix rather than a row vector when they wrote [1 2 3].

(Personally, I don't think I'll ever want to create a m-by-1 or 1-by-n matrix from scalars, but If I ever do, I'll be happy to use a more verbose syntax, such as Matrix([1 2 3]).)

JeffBezanson commented 7 years ago

There's also this:

julia> a, b = [1,2]', [3,4]'

julia> [a b]
1×4 RowVector{Int64,Array{Int64,1}}:
 1  2  3  4

julia> [a b;]
1×4 Array{Int64,2}:
 1  2  3  4
dlfivefifty commented 7 years ago

If that behaviour is maintained, then it makes no sense that [1 2] is a Matrix

StefanKarpinski commented 7 years ago

Triage decision is not to promote to matrix for scalar hcat (what @dlfivefifty said :).

andreasnoack commented 7 years ago

The thing I was trying to say during the call was that we might get some feedback when

["A" "B" "C"]

starts returning

julia> RowVector(["A", "B", "C"])
1×3 RowVector{Any,Array{String,1}}:
Error showing value of type RowVector{Any,Array{String,1}}:
ERROR: MethodError: no method matching transpose(::String)
Closest candidates are:
  transpose(::BitArray{2}) at linalg/bitarray.jl:265
  transpose(::Number) at number.jl:100
  transpose(::RowVector{T,CV} where CV<:(ConjArray{T,1,V} where V<:(AbstractArray{T,1} where T) where T) where T) at linalg/rowvector.jl:82
  ...
Stacktrace:
 [1] _getindex at ./abstractarray.jl:906 [inlined]
 [2] getindex(::RowVector{Any,Array{String,1}}, ::Int64, ::Int64) at ./abstractarray.jl:882
 [3] isassigned(::RowVector{Any,Array{String,1}}, ::Int64, ::Int64, ::Vararg{Int64,N} where N) at ./abstractarray.jl:222
 [4] alignment(::IOContext{Base.Terminals.TTYTerminal}, ::RowVector{Any,Array{String,1}}, ::Array{Int64,1}, ::Array{Int64,1}, ::Int64, ::Int64, ::Int64) at ./show.jl:1357
 [5] print_matrix(::IOContext{Base.Terminals.TTYTerminal}, ::RowVector{Any,Array{String,1}}, ::String, ::String, ::String, ::String, ::String, ::String, ::Int64, ::Int64) at ./show.jl:1487
 [6] print_matrix(::IOContext{Base.Terminals.TTYTerminal}, ::RowVector{Any,Array{String,1}}, ::String, ::String, ::String) at ./show.jl:1459
 [7] #showarray#263(::Bool, ::Function, ::IOContext{Base.Terminals.TTYTerminal}, ::RowVector{Any,Array{String,1}}, ::Bool) at ./show.jl:1709
 [8] display(::Base.REPL.REPLDisplay{Base.REPL.LineEditREPL}, ::MIME{Symbol("text/plain")}, ::RowVector{Any,Array{String,1}}) at ./REPL.jl:122
 [9] display(::Base.REPL.REPLDisplay{Base.REPL.LineEditREPL}, ::RowVector{Any,Array{String,1}}) at ./REPL.jl:125
 [10] display(::RowVector{Any,Array{String,1}}) at ./multimedia.jl:194
 [11] eval(::Module, ::Any) at ./boot.jl:235
 [12] print_response(::Base.Terminals.TTYTerminal, ::Any, ::Void, ::Bool, ::Bool, ::Void) at ./REPL.jl:144
 [13] print_response(::Base.REPL.LineEditREPL, ::Any, ::Void, ::Bool, ::Bool) at ./REPL.jl:129
 [14] (::Base.REPL.#do_respond#16{Bool,Base.REPL.##26#36{Base.REPL.LineEditREPL,Base.REPL.REPLHistoryProvider},Base.REPL.LineEditREPL,Base.LineEdit.Prompt})(::Base.LineEdit.MIState, ::Base.AbstractIOBuffer{Array{UInt8,1}}, ::Bool) at ./REPL.jl:646

i.e. a something you cannot index into.

Possible solutions seem to be

  1. Tell people to use ["A" "B" "C";]
  2. Return a Matrix (which we decided against here)
  3. Return a yet to be implemented lazy vector transpose type for non-linear algebra use
dlfivefifty commented 7 years ago

There's talk of making transpose non-recursive, which resolves this: https://github.com/JuliaLang/julia/issues/20978

See also https://discourse.julialang.org/t/proposal-rename-rowvector-to-transposevector-add-rowvector-that-doesnt-transposes-entries/4681/3

Obviously, when A and B are matrices, with the current RowVector, [A B] should be RowVector([A',B']).

Edit: that was a silly example since it should be a Matrix

andreasnoack commented 7 years ago

I'm very much aware of #20978 and I'm not too confident that it easily fixed. Instead 3. above, we could change the behavior of RowVector to be non-recursive but then we'd need a new type that is doing what RowVector is doing now so that is just a different version of 3.

StefanKarpinski commented 7 years ago

We can record here that modulo type issues due to #20978, there seems to be preference for [1 2 3] being a row vector. It doesn't seem to be crucial however, if it turns out to be impractical.

andyferris commented 7 years ago

OK, so between recursive adjoints, non-recursive transposes, the need to support all the fast BLAS stuff and lazy conjugation, the need to be able to extract a row of a Matrix as a RowVector, and this, it seems that we need to RowVectors which have different views on their elements - i.e. a RowVector which maps it's elements (lazily) through identity, conj, adjoint and x -> conj(adjoint(x)).

While I was really hoping to avoid recursive adjoints altogether, I guess we just need to do this. Similarly for the upcoming lazy matrix adjoint/transpose.

andyferris commented 7 years ago

Two design questions:

andreasnoack commented 7 years ago

Would you then apply different functions depending on the element type?

StefanKarpinski commented 7 years ago

Would it make sense to have RowVector be parameterized by a transform function? The only one above that doesn't have a standard name (and identity) is cadjoint.

andyferris commented 7 years ago

Stefan - yes exactly, we can either do that or extend the ConjArray idea. At some point someone will do (vec') .' and expect laziness so IMO in either case we still need ConjArray (plus AdjointArray and ConjAdjointArray) in Base (or else a general MappedArray).

Andreas - I'm guessing that these will all just get inlined away for e.g. Real elements so that wouldn't be strictly necessary. But we could automatically simplify these at construction time so that adjoint(::AbstractVector{<:Real}) doesn't worry about recursive adjoint or any conjugation and just chooses an identity function or whatever. But in the general case all the combinations must be tracked.

andreasnoack commented 7 years ago

Wouldn't it mean that we'd have to define hcat for every new type that would like to opt out of the recursive behavior, i.e. to use identity instead of adjoint? If so, we'd just have moved the problem to a different place.

dlfivefifty commented 7 years ago

I thought the idea is RowVector no longer transposes the entries, so hcat implementation is completely independent of adjoint she.

andreasnoack commented 7 years ago

I was referring to https://github.com/JuliaLang/julia/issues/23018#issuecomment-321738388 where it was suggested that RowVector should include a function which is applied to the elements on access.

andyferris commented 7 years ago

For hcat that function would be identity.

Though I'm thinking that extending ConjArray to AdjointArray etc to manage recursion and have no such transformation would be cleanest.

StefanKarpinski commented 6 years ago

This is a linear-algebra operation so could be decoupled from Base, but then the question is if we require doing using LinAlg before you can do [1 2 3] and get a row vector. Seems harsh.

StefanKarpinski commented 6 years ago

Triage feels that this should be a row vector.

andyferris commented 6 years ago

The plan in #23424: transpose is not a linear algebra operation (though LinAlg understands it) and RowVector is in Base not LinAlg, so this change seems consistent to me. From memory it was pretty easy to implement.

The only thing I'd really like to consider is that [1 2 3;] be a matrix... Thoughts?

StefanKarpinski commented 6 years ago

I'd be happy with that – anything with both spaces and semicolons as a matrix.

JeffBezanson commented 6 years ago

I'm ok with that too, but it's a pity there's no corresponding way to get an Nx1 object from [1, 2] (is there?). I guess that's what you get for using space as a delimiter.

Sacha0 commented 6 years ago

it's a pity there's no corresponding way to get an Nx1 object from [1, 2] (is there?)

Close enough perhaps?

julia> [1; 2]
2-element Array{Int64,1}:
 1
 2
JeffBezanson commented 6 years ago

[1; 2] and [1, 2] are both 1-d; I mean adding something to that syntax to get a 2-d, Nx1 object.

Sacha0 commented 6 years ago

Ah, sorry! My expectation that [1; 2] yields a Matrix was so strong that I did not notice it clearly returning a Vector above 😄.

andyferris commented 6 years ago

To be honest I've always been confused why [1; 2] isn't a 2x1 matrix. We'd have to change lowering of this from vcat to something else.

JeffBezanson commented 6 years ago

I would think [a; b] should concatenate two vectors, giving another vector.

andyferris commented 6 years ago

I guess I've always found this syntax meaning to be a little unnecessary when we have vcat(a, b) which is clear and easily typed, and we otherwise use semicolons for our matrix literals.

I also find that my brain assumes [...] makes an Array but the result of vcat can be anything (same for hcat and hvcat, of course).

mbauman commented 6 years ago

I must confess that this just feels slightly less obvious to me now that we no longer have a specific RowVector and instead use the more general Adjoint and Transpose wrappers. It's no longer clear to me that the proposed behavior:

julia> [1 2 3]
1×3 Transpose{Int64,Array{Int64,1}}:
 1  2  3

is any better than the status quo. I know it's semantically the same as the old RowVector… it just feels different.

fredrikekre commented 6 years ago

Also worth considering that the those wrappers likely will move out with LinAlg at some point.

JeffBezanson commented 6 years ago

As much as I like moving things out of Base, I don't think it should drive everything. If we want [1 2 3] to give a row vector, and we want that syntax available by default, then those wrappers might have to stay in Base; fine.

Sacha0 commented 6 years ago

I share Andreas's view on this question. Additionally, the matrix literal / array concatenation syntax leaves something to be desired as it stands, and adding another special case sounds less than fantastic. Best!

JeffBezanson commented 6 years ago

But what about...THIS: https://github.com/JuliaLang/julia/issues/23018#issuecomment-320081350

andyferris commented 6 years ago

So with the implementation of recursive transposed row vector, I would still love to see [“a” “b”] not throw an error (it has to make a 1x2 Matrix{String} to avoid an errors).

I mean, we could reserve this syntax for linear algebra usage and require ; as Jeff suggests for the matrix literal, I suppose.

It could potentially get a bit yuck with the storage of the elements of this literal being recursively transposed compared to how they are written - especially for element types that don’t hcat but do transpose.

Sacha0 commented 6 years ago

But what about...THIS: #23018 (comment)

That sort of oddity would be lovely to do away with in sorting out #7128. Best!

dlfivefifty commented 6 years ago

I was under the impression that the proposal was for ["a" "b"] to return RowVector(["a","b"]) and that RowVector is non-recursive (unlike Transpose and Adjoint)?

JeffBezanson commented 6 years ago

That sort of oddity would be lovely to do away with in sorting out #7128. Best!

I don't think that resolves this. Solving #7128 in general appears to be harder; indeed there is no solution in the pipeline. Currently, [a b] unambiguously means concatenation. If we changed the syntax for concatenation, the new syntax would have the same issue. Or, if we added syntax for non-concatenating matrix construction, say [| |], would [|a b|] give a matrix or a row vector?

StefanKarpinski commented 6 years ago

Triage feels that having [1 2 3] produce a plain ol' matrix is simplest and least surprising.