Skip to content

Loosen signature in triangular solver from Strided- to AbstractMatrix. #16205

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
Merged
merged 2 commits into from
May 18, 2016
Merged

Loosen signature in triangular solver from Strided- to AbstractMatrix. #16205

merged 2 commits into from
May 18, 2016

Conversation

andreasnoack
Copy link
Member

Remove many unnecessary type parameters. Fixes #16196

tkelman
tkelman reviewed May 5, 2016
View reviewed changes
base/linalg/diagonal.jl
end
end
(*)(A::AbstractTriangular, D::Diagonal) =
error("this method should never get called. Please make a bug report.")
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to define a general purpose fallback here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary to avoid an ambiguity warning but with #16125 it's not necessary anymore. I'm working on an updated version.

@kshyatt kshyatt added the linear algebra Linear algebra label May 5, 2016
@andreasnoack
Copy link
Member Author

Just rebased this PR. The test time for the triangular.jl goes from 360 seconds to 950 seconds. Second time takes about 30 seconds so it's still all about compilation. I don't know why this change makes the tests run so much slower.

@andreasnoack
Copy link
Member Author

I found it. It was caused by the removal of the At_mul_Bt method. A.'B.' therefore ended up dispatching to generic_matmatmul! which is really slow to compile. On my machine, it takes more than 0.6 seconds to compile a single version of it and here it was done for four matrix types and seven element types which is 4*7*4*7*0.6 = 470 seconds.

I think I'll try to figure out how often we are calling generic_matmatmul! without a good reason. It might also be faster if we split up the function in smaller pieces.

@andreasnoack andreasnoack force-pushed the anj/triangular branch 3 times, most recently from 5054941 to b3f7c89 Compare May 17, 2016 15:06
tkelman
tkelman reviewed May 17, 2016
View reviewed changes
base/linalg/triangular.jl
end
end

for mat in (:AbstractVector, AbstractMatrix)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is only one of these quoted?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't notice that. It doesn't give an error inside @eval.

@andreasnoack
Copy link
Member Author

Ha. Compare https://travis-ci.org/JuliaLang/julia/jobs/131233006#L1653 to https://travis-ci.org/JuliaLang/julia/jobs/131231896#L1654

From 1045 to 487 seconds just by avoiding calls to generic_matmatmul! for XTriangular.

@andreasnoack andreasnoack merged commit 1ce4ef0 into master May 18, 2016
@andreasnoack andreasnoack deleted the anj/triangular branch May 18, 2016 23:55
tkelman
tkelman reviewed May 19, 2016
View reviewed changes
base/linalg/diagonal.jl
end
end
(*)(A::AbstractTriangular, D::Diagonal) = error("this method should never be reached")
(*)(D::Diagonal, A::AbstractTriangular) = error("this method should never be reached")
Copy link
Contributor

Choose a reason for hiding this comment

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

are these only necessary to resolve ambiguity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

@tkelman tkelman mentioned this pull request Nov 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
3 participants
@andreasnoack @tkelman @kshyatt