v0.7 upgrade, comments, bugfixes and cleanup #43

mohamed82008 · 2018-08-01T10:20:52Z

This PR:

Closes Symmetric strength: should not scale cols before taking abs of values #39
Closes A no-op condition #40
Closes Classical strength should be taking max/maxabs over rows not columns #41
Closes Upgrading master to Julia v0.7 #42
Adds some comments to complicated functions such as the splitting function
Does some other cleanup here and there

ranjanan · 2018-08-01T11:18:11Z

REQUIRE

@@ -1,2 +1,3 @@
-julia 0.6
-IterativeSolvers 0.4.1
+julia 0.7


Could we still maintain v0.6 compatibility?

Maybe through Compat

It was mostly supported thanks to Compat, just needed a few @static if statements.

ranjanan · 2018-08-01T17:40:11Z

REQUIRE

@@ -1,2 +1,3 @@
 julia 0.6
-IterativeSolvers 0.4.1
+IterativeSolvers 0.7.0


We should probably use v0.6.0 because IterativeSolvers 0.7.0 needs Julia 0.7

Actually more stuff need to be changed, I was running the tests wrongly. This too yes.

Cool, ping me when you're done making all those. We should add v0.7 to .travis.yml as well. Thanks a lot for this PR! There was a lot of dead code on the classical AMG which I didn't have time to get to.

mohamed82008 · 2018-08-01T18:51:33Z

Not all deps are fully upgraded to v0.7 so they are failing the v0.7 and nightly tests. For instance, I had to use the JLD PR JuliaIO/JLD.jl#215 to make sure tests pass on my machine. Not sure what the best fix for this is, maybe allow failures and wait?

codecov-io · 2018-08-01T19:19:27Z

Codecov Report

Merging #43 into master will decrease coverage by 3.27%.
The diff coverage is 81.71%.

@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   90.85%   87.58%   -3.28%     
==========================================
  Files          10       10              
  Lines         536      604      +68     
==========================================
+ Hits          487      529      +42     
- Misses         49       75      +26

Impacted Files	Coverage Δ
src/splitting.jl	`100% <100%> (ø)`	⬆️
src/strength.jl	`98.48% <100%> (-0.03%)`	⬇️
src/aggregate.jl	`90.76% <100%> (+1.53%)`	⬆️
src/smoother.jl	`69.01% <47.05%> (-5.32%)`	⬇️
src/preconditioner.jl	`56.25% <53.33%> (-10.42%)`	⬇️
src/classical.jl	`87.96% <80.3%> (-12.04%)`	⬇️
src/utils.jl	`94.82% <81.81%> (-1.33%)`	⬇️
src/aggregation.jl	`91.66% <86.66%> (-2.57%)`	⬇️
src/multilevel.jl	`78.04% <90.56%> (+5.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 061a541...94bd0df. Read the comment docs.

ranjanan · 2018-08-01T20:12:33Z

src/AMG.jl

@@ -1,8 +1,12 @@
 module AMG


Could you also modify this module name to AlgebraicMultigrid ? This was AMG from the time it was ranjanan/AMG.jl

ranjanan · 2018-08-01T20:13:41Z

Would it help to convert all the JLD files to use JLD2 instead?

mohamed82008 · 2018-08-02T01:52:26Z

I think JLD2 is itself not upgraded yet, but moving to JLD2 seems like the right thing anyways since JLD2 seems more supported than JLD.

mohamed82008 · 2018-08-02T01:58:38Z

JLD2 also only gives warnings but tests still pass, so we are good now. The only problem now is METADATA. The url there links to this package even though it uses the old AMG.jl. Not sure how to register the new name properly. If they were 2 different repos, this advice JuliaLang/METADATA.jl#4951 (comment) may have helped. Any ideas?

mohamed82008 · 2018-08-02T02:47:00Z

v0.7 tests fail on Windows with JLD2, same error as in JuliaIO/JLD2.jl#79 (comment)

ranjanan · 2018-08-11T09:56:41Z

Hey, sorry about the late reply: JuliaCon and all. 😄

Okay so JLD and JLD2 both fail on Windows master, but the package is not directly dependent on them. If you can find any alternative method of saving this data, I'd be open to it.

mohamed82008 · 2018-08-12T20:53:35Z

I implemented a type stable parallel smoothing using KissThreading. This package is not registered yet so we cannot officially depend on it, so I commented out the code for now. I will do one last test of this package then it should be ready to review and hopefully merge.

mohamed82008 · 2018-08-12T22:32:56Z

I believe this PR is ready for a review @ranjanan.

ranjanan · 2018-08-13T08:37:50Z

Wow, this has turned out to be quite a great pull request! Thanks! 😄

I am travelling at the moment so please give me a couple days to review this.

ranjanan · 2018-08-15T13:10:56Z

src/multilevel.jl

    levels::Vector{Level{Ti,Tv}}
    final_A::SparseMatrixCSC{Ti,Tv}
    coarse_solver::S
    presmoother::Pre
    postsmoother::Post
+    workspace::TW


I like the fact that we now store this debugging information. Is there a way we can not store it by default and give the user the option to store it?

This is not for debugging, it is for pre-allocation. This is how we can do a fully inplace solve, so the ldiv! now has 0 allocations.

ranjanan · 2018-08-15T13:14:44Z

src/preconditioner.jl

+        else
+            x .= b
+        end
+        solve!(x, p.ml, b, maxiter = 1, calculate_residual = false)


maintain tol = 1e-12

Because calculate_residual is false, tol will be ignored and since tol is way too small, it will never really lead to the termination of the loop. Computing the tolerance is expensive so blindly running one iteration of the AMG solver is more efficient. Again this is what is going to happen anyways unless the residual norm is less than tol.

ranjanan · 2018-08-15T13:14:52Z

src/preconditioner.jl

+        else
+            x .= b
+        end
+        solve!(x, p.ml, b, maxiter = 1, calculate_residual = false)


ranjanan · 2018-08-15T13:15:10Z

src/preconditioner.jl

+    else
+        x = copy(b)
+    end
+    solve!(x, p.ml, b)


ranjanan · 2018-08-17T10:49:49Z

@mohamed82008 Unfortunately, there seems to be a performance regression, on Julia v0.6
Master:

Time for constructing AMG - Julia
BenchmarkTools.Trial: 
  memory estimate:  1.14 GiB
  allocs estimate:  13412189
  --------------
  minimum time:     2.234 s (34.17% GC)
  median time:      2.496 s (37.90% GC)
  mean time:        2.527 s (37.17% GC)
  maximum time:     2.851 s (38.88% GC)
  --------------
  samples:          3
  evals/sample:     1

Time for running CG - Julia
BenchmarkTools.Trial: 
  memory estimate:  1.25 GiB
  allocs estimate:  2428
  --------------
  minimum time:     4.473 s (2.39% GC)
  median time:      4.874 s (3.46% GC)
  mean time:        4.874 s (3.46% GC)
  maximum time:     5.274 s (4.37% GC)
  --------------
  samples:          2
  evals/sample:     1
sum(abs2, a * c2 - b) = 1.7685279192914618e-12

Your branch:

Time for constructing AMG - Julia
BenchmarkTools.Trial: 
  memory estimate:  3.37 GiB
  allocs estimate:  157650878
  --------------
  minimum time:     3.859 s (27.89% GC)
  median time:      3.889 s (29.20% GC)
  mean time:        3.889 s (29.20% GC)
  maximum time:     3.918 s (30.48% GC)
  --------------
  samples:          2
  evals/sample:     1

Time for running CG - Julia
BenchmarkTools.Trial: 
  memory estimate:  29.05 GiB
  allocs estimate:  1947220855
  --------------
  minimum time:     24.088 s (8.67% GC)
  median time:      24.088 s (8.67% GC)
  mean time:        24.088 s (8.67% GC)
  maximum time:     24.088 s (8.67% GC)
  --------------
  samples:          1
  evals/sample:     1
sum(abs2, a * c2 - b) = 1.7685279192914618e-12

mohamed82008 · 2018-08-17T21:50:18Z

I will look into it.

mohamed82008 · 2018-08-17T22:05:49Z

Can you post the exact code you used?

ranjanan · 2018-08-18T05:00:55Z

1m.jld.zip

using JLD, IterativeSolvers, AlgebraicMultigrid 
using BenchmarkTools
const AMG = AlgebraicMultigrid

function bench()
    a = load("/Users/ranjan/graphs/1m.jld")["G"]
    @show summary(a)
    b = zeros(size(a,1))
    b[98531] = -1
    b[332615] = 1
    ml2 = smoothed_aggregation(a)
    t2 = @benchmark smoothed_aggregation($a)
    println("Time for constructing AMG - Julia")
    display(t2)
    p2 = AMG.aspreconditioner(ml2)
    c2 = cg(a, b, Pl = p2, maxiter = 100_000, tol = 1e-6, verbose = true) 
    t2 = @benchmark x = cg($a, $b, Pl = $p2, maxiter = 100_000, tol = 1e-6) 
    println("Time for running CG - Julia")
    display(t2) 
    @show sum(abs2, a*c2 - b)
    ml
end

bench()

mohamed82008 · 2018-08-18T18:20:18Z

The performance regression in the operator building comes from making the code identical to the algorithm for unsymmetric matrices. This leads to unnecessary copying and transposing when the matrix is symmetric. Some of these can be avoided for both symmetric and unsymmetric matrices, but others are necessary for unsymmetric matrices so my solution to this is to dispatch on Symmetric to avoid unnecessary copying and transposing when the matrix is symmetric.

mohamed82008 · 2018-08-18T18:43:05Z

In other words, some extra allocations are not avoidable without being wrong for unsymmetric matrices, or at least not following the known algorithm. One can argue that the direction of being strongly coupled to some other variable is arbitrary and so we can freely swap the semantics of rows and columns, but I am not sure we should do that. I will make a correction that should make the operator building allocate less when wrapping the matrix with the Symmetric wrapper.

mohamed82008 · 2018-08-18T18:46:30Z

Oh also in v0.6 doing copy(A') allocates twice so I need to correct for that.

mohamed82008 · 2018-08-19T17:42:03Z

Interestingly the real culprit was failed inference in https://github.com/mohamed82008/AlgebraicMultigrid.jl/blob/76ac8f15e8314174bfdba559095d12a23ea5e2fe/src/smoother.jl#L29. Changing 0 to z solved the problem but I don't understand why this is a problem in the first place. Could it be a Julia bug?

mohamed82008 · 2018-08-19T17:47:08Z

Oh I see, ifelse needs to return a single type, missed that.

mohamed82008 · 2018-08-19T17:48:13Z

summary(a) = "868150×868150 SparseMatrixCSC{Float64,Int64}"
Time for constructing AMG - Julia
BenchmarkTools.Trial:
  memory estimate:  1.15 GiB
  allocs estimate:  13412264
  --------------
  minimum time:     1.484 s (34.11% GC)
  median time:      1.538 s (36.10% GC)
  mean time:        1.536 s (35.58% GC)
  maximum time:     1.584 s (35.96% GC)
  --------------
  samples:          4
  evals/sample:     1

Time for running CG - Julia
BenchmarkTools.Trial:
  memory estimate:  33.13 MiB
  allocs estimate:  295
  --------------
  minimum time:     3.011 s (0.01% GC)
  median time:      3.019 s (0.04% GC)
  mean time:        3.019 s (0.04% GC)
  maximum time:     3.027 s (0.07% GC)
  --------------
  samples:          2
  evals/sample:     1
sum(abs2, a * c2 - b) = 1.7681244199514989e-12
1.7681244199514989e-12

ranjanan · 2018-08-27T21:31:21Z

Sorry I was a little lax on this, I was moving cities. Thanks again for the great PR!

mohamed82008 added 2 commits August 1, 2018 12:02

v0.7 support, cleanup, comments and bug fixes

f40ac89

neighbour -> directed strong couplings in comments

7078229

ranjanan reviewed Aug 1, 2018

View reviewed changes

v0.6 backward compatibility

52b8322

ranjanan reviewed Aug 1, 2018

View reviewed changes

mohamed82008 added 3 commits August 1, 2018 20:04

v0.6 backward compatibility 2

2b9e314

fix travis

26ba084

fix package name in travis

22ba11e

mohamed82008 added 3 commits August 1, 2018 20:56

allow failures for v0.7 and nightly

110e1c5

allow failures 2

87cb134

allow failures 3

e812e22

ranjanan reviewed Aug 1, 2018

View reviewed changes

mohamed82008 added 4 commits August 2, 2018 03:18

AMG -> AlgebraicMultigrid

686222c

fix travis package name

ad2be73

jld -> jld2

9443fe5

add JLD2 to test REQUIRE

5248060

disallow failures for v0.7 and nightly

3913da1

Generalize direct interpol for non-symmetric matrices and edge cases

8bc5deb

mohamed82008 added 5 commits August 12, 2018 11:22

use callable structs

e89a333

jld2 -> jl files

d193468

callable Pinv and some inplace

aee7c2d

log only when log is true

cd43348

add preallocation and block support machinery

4d3929f

mohamed82008 added 4 commits August 12, 2018 17:29

allow failure on nightly

933f9b7

try v1.0 in Travis and fix nightly

ca7ba87

pre-allocate in Jacobi smoother

e15ef16

block smoothing, remove branches, and parallel Jacobi

7ed4c68

mohamed82008 added 2 commits August 12, 2018 23:30

AbstractVector -> AbstractArray and minor fixes

b519b1c

fix tests

df9db80

ranjanan reviewed Aug 15, 2018

View reviewed changes

src/preconditioner.jl

else

x .= b

end

solve!(x, p.ml, b, maxiter = 1, calculate_residual = false)

Copy link

Collaborator

ranjanan Aug 15, 2018

Choose a reason for hiding this comment

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

Same here

ranjanan reviewed Aug 15, 2018

View reviewed changes

src/preconditioner.jl Outdated

else

x = copy(b)

end

solve!(x, p.ml, b)

Copy link

Collaborator

ranjanan Aug 15, 2018

Choose a reason for hiding this comment

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

And here

Make \ call ldiv!

76ac8f1

fix v0.6 perf

c467855

fix type stability of ifelse

94bd0df

mohamed82008 force-pushed the v0.7_and_bugfixes branch from b099aeb to 94bd0df Compare August 19, 2018 17:56

ranjanan merged commit 07b586a into JuliaLinearAlgebra:master Aug 27, 2018

termi-official mentioned this pull request Jul 18, 2023

Reference for direct interpolation used #44

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v0.7 upgrade, comments, bugfixes and cleanup #43

v0.7 upgrade, comments, bugfixes and cleanup #43

mohamed82008 commented Aug 1, 2018

ranjanan Aug 1, 2018

ranjanan Aug 1, 2018

mohamed82008 Aug 1, 2018

ranjanan Aug 1, 2018

mohamed82008 Aug 1, 2018

ranjanan Aug 1, 2018

mohamed82008 commented Aug 1, 2018

codecov-io commented Aug 1, 2018 •

edited

Loading

ranjanan Aug 1, 2018

ranjanan commented Aug 1, 2018

mohamed82008 commented Aug 2, 2018

mohamed82008 commented Aug 2, 2018

mohamed82008 commented Aug 2, 2018

ranjanan commented Aug 11, 2018

mohamed82008 commented Aug 12, 2018

mohamed82008 commented Aug 12, 2018

ranjanan commented Aug 13, 2018

ranjanan Aug 15, 2018

mohamed82008 Aug 15, 2018

ranjanan Aug 15, 2018

mohamed82008 Aug 15, 2018

ranjanan Aug 15, 2018

ranjanan Aug 15, 2018

ranjanan commented Aug 17, 2018 •

edited

Loading

mohamed82008 commented Aug 17, 2018

mohamed82008 commented Aug 17, 2018

ranjanan commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 19, 2018

mohamed82008 commented Aug 19, 2018

mohamed82008 commented Aug 19, 2018

ranjanan commented Aug 27, 2018

v0.7 upgrade, comments, bugfixes and cleanup #43

v0.7 upgrade, comments, bugfixes and cleanup #43

Conversation

mohamed82008 commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamed82008 commented Aug 1, 2018

codecov-io commented Aug 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ranjanan commented Aug 1, 2018

mohamed82008 commented Aug 2, 2018

mohamed82008 commented Aug 2, 2018

mohamed82008 commented Aug 2, 2018

ranjanan commented Aug 11, 2018

mohamed82008 commented Aug 12, 2018

mohamed82008 commented Aug 12, 2018

ranjanan commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranjanan commented Aug 17, 2018 • edited Loading

mohamed82008 commented Aug 17, 2018

mohamed82008 commented Aug 17, 2018

ranjanan commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 18, 2018

mohamed82008 commented Aug 19, 2018

mohamed82008 commented Aug 19, 2018

mohamed82008 commented Aug 19, 2018

ranjanan commented Aug 27, 2018

codecov-io commented Aug 1, 2018 •

edited

Loading

ranjanan commented Aug 17, 2018 •

edited

Loading