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

Label QRCompactWY.T as Triangular #7649

Merged
merged 6 commits into from
Jul 30, 2014
Merged

Label QRCompactWY.T as Triangular #7649

merged 6 commits into from
Jul 30, 2014

Conversation

jiahao
Copy link
Member

@jiahao jiahao commented Jul 18, 2014

Fixes JuliaLang/LinearAlgebra.jl#122

This PR currently does fix the original issue, but in order to do so, I had to reorder the way things are loaded in LinAlg so that Triangular in triangular.jl was defined before QRCompactWY in factorization.jl. This now throws the ambiguity warning:

linalg.jl
Warning: New definition 
    A_mul_Bc could not show value of type Tuple at linalg/factorization.jl:370
is ambiguous with: 
    A_mul_Bc could not show value of type Tuple at linalg/triangular.jl:185.
To fix, define 
    A_mul_Bc could not show value of type Tuple
before the new definition.

when building Julia.

Would appreciate any help to resolve the warning.

@ViralBShah ViralBShah added this to the 0.3 milestone Jul 18, 2014
@ViralBShah
Copy link
Member

Rebased this on master.

@ViralBShah
Copy link
Member

This warning looks a little puzzling. Cc: @JeffBezanson @andreasnoackjensen

Also, I wonder if it should be AbstractMatrix instead of AbstractArray for the overloaded multiplication with QR objects at line 370 of factorization.jl.

@JeffBezanson
Copy link
Member

Fixed the warning, but still have this test failure:

exception on 2: ERROR: `QRCompactWYQ{S}` has no method matching QRCompactWYQ{S}(::Array{Float32,2}, ::Triangular{Float32,Array{Float32,2},:U,false})
 in getindex at linalg/factorization.jl:203
 in anonymous at no file:115
 in runtests at /home/jeff/src/julia2/julia/test/testdefs.jl:5
 in anonymous at multi.jl:847
 in run_work_thunk at multi.jl:613
 in anonymous at task.jl:847
while loading linalg1.jl, in expression starting on line 25

Should QRCompactWYQ.T also be Triangular?

@jiahao
Copy link
Member Author

jiahao commented Jul 22, 2014

Should QRCompactWYQ.T also be Triangular?

Yes

@andreasnoack
Copy link
Member

Is it enough to specify Triangular{S} when the type has four parameters? If yes, why? Regarding the AbstractArray, it might be because the signature predates the definition of AbstractVecOrMat.

@ViralBShah
Copy link
Member

It should be ok to specify Triangular{S}, just like we often do only Array{T}. Good to know that we can replace the AbstractArray with AbstracVecOrMat.

jiahao and others added 6 commits July 29, 2014 21:43
This doesn't build because Triangular isn't defined yet when
factorization is built.
Note: produces ambiguity warning at compile time
Warning: New definition
    A_mul_Bc could not show value of type Tuple at
linalg/factorization.jl:370
is ambiguous with:
    A_mul_Bc could not show value of type Tuple at
linalg/triangular.jl:180.
To fix, define
    A_mul_Bc could not show value of type Tuple
before the new definition.
jiahao added a commit that referenced this pull request Jul 30, 2014
Label QRCompactWY.T as Triangular
@jiahao jiahao merged commit 8d4f989 into master Jul 30, 2014
@jiahao jiahao deleted the cjh/fix-7304 branch July 30, 2014 03:09
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.

Label QRCompactWY.T as Triangular
4 participants