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

Fix type inference #16

Merged
merged 2 commits into from
Feb 24, 2021
Merged

Fix type inference #16

merged 2 commits into from
Feb 24, 2021

Conversation

frapac
Copy link
Member

@frapac frapac commented Feb 15, 2021

This PR allows to improve type inference in ExaTron. The signature of ExaTronProblem has been updated, from

mutable struct ExaTronProblem{VI, VD}

to

mutable struct ExaTronProblem{VI, VD, TM <: AbstractTronMatrix}

This allows Julia to infer directly the type of the matrix we are using, instead of dealing with unspecified AbstractTronMatrix.

Also:

  • the preconditioner has been updated accordingly (we now specified the preconditioner directly in the option, :ICFS by default)
  • we have fixed important type inference issue with TronDenseMatrix, leading to nice speed-up

A benchmark (using profiling/qptests.jl) gives:

  • for TronSparseMatrixCSC, we reduce the overall number of allocations (now the function dtron is not allocating anything, which was not the case before, because of type inference issue)
#This PR
  369.747 μs (54 allocations: 2.36 KiB)
# main 
  386.941 μs (107 allocations: 3.59 KiB)
  • for TronDenseMatrix{MT}, we get almost a 300x speed-up:
#This PR
  90.673 ms (69 allocations: 2.64 KiB)
# main 
  26.026 s (926692690 allocations: 13.81 GiB)

Fix #10

@frapac frapac requested a review from youngdae February 15, 2021 11:45
@@ -32,9 +33,9 @@ function TronDenseMatrix(I::VI, J::VI, V::VD, n) where {VI, VD}
@assert length(I) == length(J) == length(V)

if isa(V, Array)
A = TronDenseMatrix{Array}(n, n, tron_zeros(Array{eltype(V)}, (n, n)))
A = TronDenseMatrix{Array{Float64, 2}}(n, n, tron_zeros(Array{eltype(V)}, (n, n)))
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to specify explicitly the signature of the Array here, if we want to compile on Array{Float64, 2} instead of the more generic type Array

@youngdae youngdae merged commit 557408e into main Feb 24, 2021
@youngdae
Copy link
Member

Looks great! (Sorry for the delayed merging)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specifying explicit type information for AbstractTronMatrix
2 participants