-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add convert()
methods for Matrix
from Vector
and RowVector
#21977
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
Conversation
base/array.jl
Outdated
@@ -320,6 +320,8 @@ oneunit(x::AbstractMatrix{T}) where {T} = _one(oneunit(T), x) | |||
|
|||
convert(::Type{Vector}, x::AbstractVector{T}) where {T} = convert(Vector{T}, x) | |||
convert(::Type{Matrix}, x::AbstractMatrix{T}) where {T} = convert(Matrix{T}, x) | |||
convert(::Type{Matrix}, x::Vector{T}) where {T} = Matrix(reshape(x, :, 1)) | |||
convert(::Type{Matrix}, x::RowVector{T,S}) where {T,S} = Matrix(reshape(x, 1, :)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be constructors instead of converts since I don't think we want these to happen implicitly. Also I think these should not use reshape
since the vector will not be resizeable afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the right way to do this without using reshape()
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allocate and copy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuyichao I'm trying to figure out what you mean by the vector is not resizeable after a reshape()
:
julia> x = Vector(1:4)
4-element Array{Int64,1}:
1
2
3
4
julia> y = reshape(x, 2, 2)
2×2 Array{Int64,2}:
1 3
2 4
julia> z = reshape(x, 1, 4)
1×4 Array{Int64,2}:
1 2 3 4
julia> w = reshape(y, 1, 4)
1×4 Array{Int64,2}:
1 2 3 4
Everything seems to "just work". What makes something not resizeable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He means combinations of reshape
and resize!
behave funny (sorry there's a bit of OhMyREPL going on below):
julia 0.5> x = Vector(1:4)
4-element Array{Int64,1}:
1
2
3
4
julia 0.5> y = reshape(x, 2, 2)
2×2 Array{Int64,2}:
1 3
2 4
julia 0.5> resize!(x, 5)
------ ErrorException ------------------ Stacktrace (most recent call last)
[1] — resize!(::Array{Int64,1}, ::Int64) at array.jl:512
cannot resize array with shared data
The implementation of Array
is a bit "special". While it's always been semantically allowed to resize a vector, a reshaped array will by default be a view of the original data. So if you have two views of the same data, and resize one, you want to guard against the fact the second data semantically hasn't been resized. Either we make this an error or else sometimes transparently make a copy in circumstances that the author of a local method can't predict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, I see. Well in the new code that I've pushed on this branch I don't use reshape()
at all, which is probably the right thing to do, because you wouldn't want a convert()
call to be a view into the original data anyway, I don't think.
The test should also include resizability of the Vector after calling this. |
c25cb56
to
e69eb08
Compare
Is My suggestion is something along the lines of |
Convert is fine, IMO: there's a very clear and consistent identification between vectors and column matrices, row vectors and row matrices. Converting back-and-forth is perfectly reasonable. |
Arguably, we should also have the reverse conversions from column matrix to vector and row matrix to row vector, which throws an error in the former case if the number of columns is not one, and in the latter case if the number of rows is not one. |
I wonder what the relationship between this kind of behavior and promotion of types is. E.g. could we view vectors and matrices like integers and floating-point numbers, and just define type promotion rules that allow automatic "widening" from a column vector to a Matrix? The reason I ask is because it seems much more nuanced to automatically convert a Matrix to a Vector. |
We already do, effectively: julia> [1,2,3] + hcat([1,2,3])
3×1 Array{Int64,2}:
2
4
6
julia> [1,2,3]' + [1 2 3]
1×3 Array{Int64,2}:
2 4 6 This is the reason I'm in favor of this. We already make this identification in so many places – and it would be awkward and impractical not to – so the way to make things consistent is to take it all the way and make the identification work everywhere it could. |
Yes, I also think |
The other complication is recursive transpose - when converting from (Somewhere else there is a suggestion to remove recursive transpose (only make |
Of course, incremental improvements are better than none (so this PR is a good idea), but I feel it'd be only half-finished if there isn't an |
e69eb08
to
aa50086
Compare
This sounds close to what we're talking about in #21998, but I'm not confident in my understanding of what you're talking about. Could you spell it out for me a bit more? |
Yep - I think it would be useful to have a single function that can:
More-or-less, I think |
@andyferris I think that's a bigger concern than I want to tackle with this PR. So I vote that we get the limited functionality here merged as-is, then move on to the more fundamental questions in a separate work. |
Sure, very reasonable! I mainly mentioned the general case in case that solution would obviate the desire for these methods. But at this stage I don't see an argument for not having both. |
@yuyichao have I addressed/dodged all your comments? |
@andyferris @mbauman after all the discussion about how equality between these kinds of types should work, is this PR still desireable? |
Now that we take vector transposes seriously, it makes sense to provide these simple conversion routines from
Vector
/RowVector
toMatrix
.I'm definitely open to suggestions on the most efficient way to do this; I don't know the array subsystem very well.