Skip to content

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

Merged
mbauman merged 3 commits intoJuliaLang:masterfrom
Jutho:patch-2
Aug 22, 2018
Merged

Make copyto!(A,I) work for rectangular matrices#28790
mbauman merged 3 commits intoJuliaLang:masterfrom
Jutho:patch-2

Conversation

@Jutho
Copy link
Copy Markdown
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
Copy Markdown
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
@Jutho
add tests for copyto!
5cbc9e1 
add tests for `copyto!(::Matrix, ::UniformScaling)`
@Jutho
fix whitespace
7dc01c3
Sacha0
Sacha0 approved these changes Aug 22, 2018
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@mbauman mbauman mentioned this pull request Aug 22, 2018
Merged
@Jutho Jutho deleted the patch-2 branch August 23, 2018 08:06
staticfloat pushed a commit that referenced this pull request Aug 24, 2018
@Jutho @staticfloat
Make copyto!(A,I) work for rectangular matrices (#28790)
314247f 
* 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
@mortenpi
News and compat for JuliaLang#28790
35484c2 
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
@mortenpi
News and compat annotation for JuliaLang#28790
970a3d7 
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
@mortenpi
News and compat for JuliaLang#28790 copyto!(::AbstractMatrix, ::Unifo…
3676001 
…rmScaling)
fredrikekre pushed a commit that referenced this pull request Dec 1, 2018
@mortenpi @fredrikekre
News and compat for #28790 copyto!(::AbstractMatrix, ::UniformScaling)
e63f465
fredrikekre pushed a commit that referenced this pull request Dec 1, 2018
@mortenpi @fredrikekre
News and compat for #28790 copyto!(::AbstractMatrix, ::UniformScaling)
b1ad9eb
fredrikekre pushed a commit that referenced this pull request Dec 3, 2018
@mortenpi @fredrikekre
News and compat for #28790 copyto!(::AbstractMatrix, ::UniformScaling)
95f0acd
fredrikekre pushed a commit that referenced this pull request Dec 4, 2018
@mortenpi @fredrikekre
News and compat for #28790 copyto!(::AbstractMatrix, ::UniformScaling)
7d5ff0f
fredrikekre pushed a commit that referenced this pull request Dec 4, 2018
@mortenpi @fredrikekre
News and compat for #28790 copyto!(::AbstractMatrix, ::UniformScaling)
67a1bd2
fredrikekre added a commit that referenced this pull request Dec 5, 2018
@mortenpi @fredrikekre
Addition of NEWS and compat admonitions for important
cceb675 
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 <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
@mortenpi @fredrikekre
Addition of NEWS and compat admonitions for important
ebbabf5 
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 <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
@fredrikekre @mortenpi
Compat admonitions and NEWS for Julia 1.1 (#30230)
6175bd9 
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 <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Sacha0 Sacha0 Sacha0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

linear algebra Linear algebra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Jutho @Sacha0 @mbauman @fredrikekre