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 (c)transpose work correctly for block matrices #7244

Merged
merged 1 commit into from
Jun 17, 2014
Merged

Conversation

andreasnoack
Copy link
Member

With this change, the matrix Apd generated by

julia> A=[randn(2,2) for i = 1:2,j=1:2]
2x2 Array{Array{Float64,2},2}:
 2x2 Array{Float64,2}:
 0.779435  -0.145332
 2.06874   -1.24279   …  2x2 Array{Float64,2}:
 0.036242  0.40966 
 0.584986  0.122122
 2x2 Array{Float64,2}:
  0.471302  0.577977
 -1.77892   0.216775     2x2 Array{Float64,2}:
 -1.17572  0.296942
  1.41572  0.535884

julia> Apd = A'A;

will be positive definite.

@JeffBezanson
Copy link
Sponsor Member

@jiahao @stevengj Any reason not to merge this?

@StefanKarpinski
Copy link
Sponsor Member

This seems to me like the right thing to do.

@andreasnoack
Copy link
Member Author

Oh. I didn't run the string tests locally. I'll define transpose(s::String)=s.

@stevengj
Copy link
Member

lgtm

@JeffBezanson
Copy link
Sponsor Member

Very related: #6395
Defining transpose just for strings seems really arbitrary. To make this work, it looks like we will have to go through with the plan in #6395 to make conj and ctranspose fall back to the identity function for everything.

@JeffBezanson
Copy link
Sponsor Member

Or, I guess

transpose(x) = x
ctranspose(x) = conj(transpose(x))
conj(x) = x

@StefanKarpinski
Copy link
Sponsor Member

I know it seems a bit sketch, but I think this is the way to go. Let's try it out and see what comes of it.

@andreasnoack
Copy link
Member Author

I'll do that for now, but defining methods for Any always feels dangerous. There are no transpose methods for AbstractMatrix so for new matrix types this would give the wrong result. Any is so wild so maybe something like Scalar<:Any with Number<:Scalar and String<:Scalar would make sense. Alternative names could be Element or Atom, but the name is not the important part here.

@JeffBezanson
Copy link
Sponsor Member

I have sometimes wondered if we should add Atom (above Symbol, Number, etc.) and Collection (above AbstractArray, Associative, ...) abstract types.

This is a bit sketch, but less so than making it work only for strings.

@andreasnoack
Copy link
Member Author

Okay. I have added line definitions to operators.jl.

@StefanKarpinski
Copy link
Sponsor Member

I like Atom and Collection, but which does a String qualify as?

@JeffBezanson
Copy link
Sponsor Member

Yep, that's one reason I didn't add those types :) You don't necessarily want to be forced to classify everything that way.

@ViralBShah
Copy link
Member

I like this one.

@jiahao
Copy link
Member

jiahao commented Jun 17, 2014

lgtm too

andreasnoack added a commit that referenced this pull request Jun 17, 2014
Make (c)transpose work correctly for block matrices
@andreasnoack andreasnoack merged commit 4cfb9a1 into master Jun 17, 2014
@andreasnoack andreasnoack deleted the anj/block branch June 17, 2014 18:37
mauro3 pushed a commit to mauro3/julia that referenced this pull request Jun 19, 2014
@mbauman mbauman mentioned this pull request Sep 16, 2015
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants