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

constructors for Matrix and SparseMatrixCSC from UniformScaling #24372

Merged
merged 4 commits into from
Oct 31, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 28, 2017

This pull request provides constructors for Matrix and SparseMatrixCSC from UniformScalings, for example Matrix(I, dims) and SparseMatrixCSC(I, dims). In other words, this pull request explores the intersection of #11557 (particularly #11557 (comment)) and #23156. So far this approach feels nice, supporting the direction in #11557 (comment). Best!

@Sacha0 Sacha0 added the arrays [a, r, r, a, y, s] label Oct 28, 2017
@Sacha0 Sacha0 force-pushed the constructI branch 5 times, most recently from aaf3582 to 8fd2325 Compare October 28, 2017 17:06
@Sacha0 Sacha0 added the triage This should be discussed on a triage call label Oct 28, 2017
@Sacha0 Sacha0 changed the title [WIP] constructors for Matrix and SparseMatrixCSC from UniformScaling constructors for Matrix and SparseMatrixCSC from UniformScaling Oct 28, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 30, 2017

Ref. #23156 (comment), suggesting special-casing Matrix(I, m, n)/sparse(I, m, n)/SparseMatrixCSC(I, m, n) to yield an array with eltype Float64 rather than eltype(I).

@andreasnoack
Copy link
Member

As mentioned in #23156, I don't think it is necessary to return Float64 elements. Using Int elements will probably also result in a better promotion in some cases. At least for Float32 computations.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 30, 2017

Cleaned up the existing commits and added a couple commits providing the convenience forms suggested by @andreasnoack's #23156 (comment) (e.g. Matrix[{T}](I, n), SparseMatrixCSC[{T}](I, n)).

This pull request should be in shape. Absent objections or requests for time, I am inclined to move forward with this approach (merge) in the next day or two. Thoughts? Thanks!

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 31, 2017

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants