Increase performance via symmetry of Hessian #151
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I would say this is breaking, yes. Maybe we could just do sparse(Symmetric(sparse(...))) at the very end, that's very fast |
Should we make the change with our next breaking release though?
I've handled it manually. :) |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 91.92% 91.94% +0.02%
==========================================
Files 38 38
Lines 1498 1503 +5
==========================================
+ Hits 1377 1382 +5
Misses 121 121 ☔ View full report in Codecov by Sentry. |
Benchmark resultJudge resultBenchmark Report for /home/runner/work/SparseConnectivityTracer.jl/SparseConnectivityTracer.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineTarget resultBenchmark Report for /home/runner/work/SparseConnectivityTracer.jl/SparseConnectivityTracer.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoBaseline resultBenchmark Report for /home/runner/work/SparseConnectivityTracer.jl/SparseConnectivityTracer.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoRuntime information
|
|
I've never seen a single Hessian result appear in any of the hundreds of Benchmark results we've produced over the years. Maybe they get discarded because they get run with too few samples? I can't believe this PR didn't result in a performance improvement. |
|
If you unfold and |
|
Even weirder, when you check the CI logs, the Hessian benchmarks also run for the baseline branch |
|
I get a decent speed-up when running the This PRjulia> include("benchmark/hessian.jl")
hessbench_randomsparsity (generic function with 1 method)
julia> method = TracerSparsityDetector()
TracerSparsityDetector{SparseConnectivityTracer.GradientTracer{SparseConnectivityTracer.IndexSetGradientPattern{Int64, BitSet}}, SparseConnectivityTracer.HessianTracer{SparseConnectivityTracer.IndexSetHessianPattern{Int64, BitSet, Set{Tuple{Int64, Int64}}, false}}}()
julia> N, K = 200, 16
(200, 16)
julia> x = rand(N);
julia> f = ArrowHead(K)
ArrowHead(16)
julia> @benchmark hessian_sparsity($f, $x, $method)
BenchmarkTools.Trial: 389 samples with 1 evaluation.
Range (min … max): 11.577 ms … 16.529 ms ┊ GC (min … max): 0.00% … 23.09%
Time (median): 12.855 ms ┊ GC (median): 8.48%
Time (mean ± σ): 12.845 ms ± 548.286 μs ┊ GC (mean ± σ): 7.83% ± 3.41%
▆▆▆▆▅▆█▁▂▁▃▂
▂▄▄▆▄▇▄▄▃▃▃▁▃▂▁▁▁▂▁▂▄▄▆▄▆████████████▇▄▆▄▂▃▄▃▃▂▄▃▃▂▂▄▂▁▄▂▂▃▄ ▄
11.6 ms Histogram: frequency by time 14.1 ms <
Memory estimate: 42.39 MiB, allocs estimate: 98757.
|
It looks like the report summarizes the baseline as well, but for some reason doesn't put it in the summary table. A speedup is clearly visible:
|
This was referenced Aug 12, 2024
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #34.
By only keeping track of index-tuples$(i, j)$ with $i≤j$ , the size of all "Hessian-sets" is reduced by roughly half.
As seen in #135, the size of sets plays a large role in the performance of SCT as it affects the performance of union operations.
This PR changes the output type ofhessian_sparsityfromSparseMatrixCSC{Bool, Int64}toSymmetric{Bool, SparseArrays.SparseMatrixCSC{Bool, Int64}}.Is this (a) desired and (b) should this be considered breaking? (Ping @gdalle for the matrix coloring perspective)