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

Make copyto!(A,I) work for rectangular matrices #28790

Merged
merged 3 commits into from
Aug 22, 2018
Merged

Conversation

Jutho
Copy link
Contributor

@Jutho Jutho commented Aug 21, 2018

Given that Matrix{T}(I, (m,n)) works for rectangular matrices m != n, I would also expect copyto!(A,I) to work. I often use this to initialize an existing matrix with ones on the diagonal, even if it is rectangular.

An example is in LinearAlgebra, suppose you want to materialize the q factor in a QR decomposition. You might have two equally sized matrices A and B lying around:

using LinearAlgebra
Q,R = qr!(A)
Qmaterialized = lmul!(Q, copyto!(B,I))

I'll probably need to update some tests as well?

@Sacha0
Copy link
Member

Sacha0 commented Aug 21, 2018

I'll probably need to update some tests as well?

Tests would be lovely! It seems the UniformScaling tests lack anything for copyto! presently? Best!

@mbauman mbauman added needs tests Unit tests are required for this change linear algebra Linear algebra labels Aug 21, 2018
Jutho added 2 commits August 22, 2018 09:17
add tests for `copyto!(::Matrix, ::UniformScaling)`
Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

Thanks @Jutho! :)

@Jutho
Copy link
Contributor Author

Jutho commented Aug 22, 2018

You're welcome. I assume the one travis error is unrelated? Sorry about the whitespace error at first; I edited online using Github, and indenting a blank line the same as the surrounding code lines seems to be the default.

@fredrikekre fredrikekre removed the needs tests Unit tests are required for this change label Aug 22, 2018
@mbauman
Copy link
Member

mbauman commented Aug 22, 2018

Hah, funny, yes, that test failure is unrelated. Looks like it's the totally_not_five26034 test struct that introduces ambiguities… and it'll only fail if that same worker ends up doing the linalg tests.

@mbauman mbauman merged commit 6a2dcc6 into JuliaLang:master Aug 22, 2018
@Jutho Jutho deleted the patch-2 branch August 23, 2018 08:06
staticfloat pushed a commit that referenced this pull request Aug 24, 2018
* Make copyto!(A,I) work for rectangular matrices

* add tests for `copyto!`

add tests for `copyto!(::Matrix, ::UniformScaling)`

* fix whitespace
mortenpi added a commit to mortenpi/julia that referenced this pull request Dec 1, 2018
Add a docstring for copyto!(::AbstractMatrix, ::UniformScaling) and
document 1.1 changes.
mortenpi added a commit to mortenpi/julia that referenced this pull request Dec 1, 2018
Add a docstring for copyto!(::AbstractMatrix, ::UniformScaling) and
document 1.1 changes.
mortenpi added a commit to mortenpi/julia that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants