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.

Sign up for GitHub

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

Jump to bottom

HermOrSym should preserve structure when scaled with Numbers. #29469

Merged
merged 1 commit into from
Oct 19, 2018
Merged

HermOrSym should preserve structure when scaled with Numbers. #29469

merged 1 commit into from
Oct 19, 2018

Conversation

fredrikekre
Copy link
Member

Changes the output type of some operations so not sure we can backport this...

@fredrikekre fredrikekre added linear algebra Linear algebra minor change Marginal behavior change acceptable for a minor release labels Oct 2, 2018
@StefanKarpinski StefanKarpinski added this to the 1.1 milestone Oct 2, 2018
Sacha0
Sacha0 reviewed Oct 2, 2018
View reviewed changes
stdlib/LinearAlgebra/test/symmetric.jl
@test (2.0 * H)::Matrix{ComplexF64} == (H * 2.0)::Matrix{ComplexF64}
@test (S / 2.0)::Matrix{Float64} == Matrix(S) / 2.0
@test (H / 2.0)::Matrix{ComplexF64} == Matrix(H) / 2.0
end
Copy link
Member

Choose a reason for hiding this comment

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

I imagine I'm missing something obvious: Why not preserve the structural information? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we trying to make Symmetric/Hermitian full fledged abstract arrays? I feel like that can never be complete, there will always be something that does not return a Symmetric. Personally I just consider them dispatch wrappers, and same with Adjoint/Transpose, but I guess I am alone in this thinking.

Anyway, I guess this change was not necessary and maybe it is better to make ::Number * ::SymOrHerm preserve the structural property?

Copy link
Member

@StefanKarpinski StefanKarpinski Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like that can never be complete

There are a finite number of operations defined in Base, so this can't be true. We're still in the phase where we're encountering things we missed all the time, but it's getting less and less over time and will eventually be none. If it's still a common problem in a couple of minor versions (say 1.3 or so) then, we can consider if it's just not working, but it seems premature to declare failure at this point.

andreasnoack
andreasnoack reviewed Oct 17, 2018
View reviewed changes
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.

  1. I don't see how this is related to SymOrHerm * Number returns different type than Numer * SymOrHerm #29392. What am I missing?
  2. What is the motivation for this? Generally, I think it's a good idea to preserve structure when possible.

@fredrikekre
Copy link
Member Author

Updated.

@fredrikekre fredrikekre changed the title fix #29392: correct ambiguity resolving fallback for of (*|/)(::Number, ::SymOrHerm) HermOrSym should preserve structure when scaled with Numbers. Oct 18, 2018
@fredrikekre fredrikekre merged commit c71b76a into master Oct 19, 2018
@fredrikekre fredrikekre deleted the fe/fix29392 branch October 19, 2018 06:25
fredrikekre added a commit that referenced this pull request Nov 30, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
25ca508
fredrikekre added a commit that referenced this pull request Dec 1, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
6876b34
fredrikekre added a commit that referenced this pull request Dec 1, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
ebc7931
fredrikekre added a commit that referenced this pull request Dec 3, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
efe672f
fredrikekre added a commit that referenced this pull request Dec 4, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
77d4910
fredrikekre added a commit that referenced this pull request Dec 4, 2018
@fredrikekre
News for #29469 (HermOrSym preserve wrapper when scaled with Number).
a04d13f
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

@Sacha0 Sacha0 Sacha0 left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@andreasnoack andreasnoack andreasnoack approved these changes

Assignees
No one assigned
Labels
linear algebra Linear algebra minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@fredrikekre @StefanKarpinski @andreasnoack @Sacha0