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

Add matrix, an operation to convert an array to a matrix (like vec) #23790

Closed
wants to merge 3 commits into from

Conversation

oxinabox
Copy link
Contributor

We have vec which is a short-hand for reshape(x, Val(1)).
I propose that we should have mat as well, as a shorthand for reshape(x,Val(2)).

Obviously vec is the most common case of reshaping,
but I suggest that mat comes in second, and is much more common than the general reshape.
In particular for converting a Vector into a single column matrix.
Very common for interfacing with various libraries.

My old trick for that was to double transpose it (x''), but since as of 0.6 we take vector transpose seriously, that won't work. (Plus this way is nonallocating).

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

This would be useful to have. I think I'd prefer matrix instead of mat even though mat is similar to vec. However, vec is standard whereas mat is not and I prefer to spell things out.

"""
vec(a::AbstractArray) = reshape(a,_length(a))
vec(a::AbstractVector) = a


"""
mat(a::AbstractArray) -> Matrix
Copy link
Member

Choose a reason for hiding this comment

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

Should be AbstractMatrix instead of Matrix.

@oxinabox
Copy link
Contributor Author

Makes sense to spell it out. mat is pretty vague.
I liked the 3 letter match to vec, but explict is better.
I'll fix the doc on vec too w.r.t Abstract

@oxinabox oxinabox changed the title Add mat, an operation to convert an array to a matrix Add matrix, an operation to convert an array to a matrix (like vec) Sep 20, 2017
@GunnarFarneback
Copy link
Contributor

Would it be reasonable to expect this to convert a RowVector to a 1xN matrix? The matrix(x')' workaround isn't that attractive.

@oxinabox
Copy link
Contributor Author

Would it be reasonable to expect this to convert a RowVector to a 1xN matrix? The matrix(x')' workaround isn't that attractive.

What do you mean?
A RowVector is a 1xN AbstractMatrix already.
If you want to convert it to a 1xN concrete Matrix, use Matrix, or convert(Matrix, x)

julia> supertype(RowVector) == AbstractMatrix
true

julia> x = RowVector([1,2,3])
1×3 RowVector{Int64,Array{Int64,1}}:
 1  2  3

julia> size(x)==(1,3)
true

julia> matrix(x)
1×3 RowVector{Int64,Array{Int64,1}}:
 1  2  3

julia> matrix(x) == x
true


julia> Matrix(x)
1×3 Array{Int64,2}:
 1  2  3

@GunnarFarneback
Copy link
Contributor

Yes, I wanted a concrete Matrix without copying the data. Maybe I should just look the other way to make the receivers of the data more open to abstract matrices.

@andyferris
Copy link
Member

I like the direction. There are a bunch of cases where you want to cast your quasi-1d collection into a specific shape, so that's cool.

I totally haven't got around to this, but I was hoping to rather have this little family of functions:

  • vec (as now)
  • rowvec (basically rowvec(x) = vec(x).' with non-recursive transpose)
  • rowmat (never makes a RowVector)
  • colmat

since sometimes you want a column matrix and sometimes a row matrix.

@GunnarFarneback
Copy link
Contributor

There's still something bothering me with the pseudomatrixness of RowVectors. If I say matrix(x) I would expect to get something that is matrixy enough that I can transpose it and always have a matrix, not something that occasionally reverts to a vector.

@oxinabox
Copy link
Contributor Author

oxinabox commented Oct 19, 2017

@andreasnoack is this good to go now?
As far as satisfying your review

Should I add @andyferris 's stuff?
@andyferris so colmat === matrix as currently defined, right?

@andyferris
Copy link
Member

The same, yes.

@GunnarFarneback I agree. matrix or rowmat or whatever should never let the RowVector wrapper survive.

@andreasnoack
Copy link
Member

Now that we have a RowVector I think Vectors are effectively columns so I think it would be fine to make matrix(Vector) return a column matrix and matrix(RowVector) return a row matrix.

@Sacha0
Copy link
Member

Sacha0 commented Nov 5, 2017

With the present direction of array constructors, Matrix constructors could eventually do the trick? And perhaps likewise with vec, Vector constructors? Best!

@mbauman
Copy link
Member

mbauman commented Nov 6, 2017

I don't think constructors can fill this role since I'd expect them to copy the data; vec and reshapes are views. Maybe #23821?

Of course, convert(AbstractMatrix, x) is even more verbose than reshape, so the primary motivation here still remains.

@Sacha0
Copy link
Member

Sacha0 commented Nov 6, 2017

I don't think constructors can fill this role since I'd expect them to copy the data; vec and reshapes are views.

Ah, right! Thank you :).

Maybe #23821?

👍

@oxinabox
Copy link
Contributor Author

oxinabox commented Nov 7, 2017

@andyferris's set of functions is progressively growing on me.

@jebej
Copy link
Contributor

jebej commented Nov 8, 2017

rowvec (basically rowvec(x) = vec(x).' with non-recursive transpose)

@andyferris Shouldn't that operation go row major, i.e. rowvec(x) = vec(x.')

@andyferris
Copy link
Member

@jebej I wouldn't have thought so... the idea here is to turn any iterable into a RowVector such that iteration is preserved. If users want to change the iteration ordering to row major they should do that explicitly with .'.

@oxinabox
Copy link
Contributor Author

I think we can close this. We have matrix literals even for matrixes of length 1, and we have reshape.

@oxinabox oxinabox closed this Jul 29, 2023
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