-
Notifications
You must be signed in to change notification settings - Fork 11
Fix the inference of the function coloring for static compilation #340
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,47 @@ | ||||||||||||||||||||||||||||||||||||||||||
| # Guard for static compilation (`juliac --trim=safe`). | ||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering why our existing type stability tests didn't catch this: it's because we didn't pass the SparseMatrixColorings.jl/test/type_stability.jl Lines 38 to 57 in a8fb3c4
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch Guillaume! Test Summary: | Pass Fail Error Broken Total Time
SparseMatrixColorings | 64392 66 33 31 64522 12m01.6s
Code quality | 13 13 2m48.7s
Aqua | 11 11 2m23.4s
JET | 1 1 19.1s
Doctests | 1 1 6.2s
Internals | 9269 24 9293 1m10.2s
Graph | 8068 8068 5.2s
Forest | 20 20 0.5s
Order | 526 526 19.7s
Check | 70 70 7.1s
Matrices | 20 20 1.6s
Constructors | 7 7 0.0s
Result | 483 483 0.9s
Constant coloring | 18 18 0.4s
Optimal coloring | 1 24 25 23.0s
ADTypes coloring algorithms | 10 10 3.5s
Visualization | 46 46 8.1s
Correctness | 54200 5 54205 4m39.2s
Small instances | 3008 3008 1m26.0s
Random instances | 47256 47256 1m32.8s
Structured matrices | 3627 3627 24.4s
Instances with known colorings | 12 12 1.1s
SuiteSparse | 297 5 302 1m14.8s
Performance | 910 66 33 2 1011 3m21.2s
Type stability | 855 66 33 954 2m52.9s
Sparse coloring | 69 66 33 168 49.5s
ADTypes | 3 3 6.2s
nonsymmetric - column - direct | 22 22 4.8s
nonsymmetric - row - direct | 22 22 2.9s
symmetric - column - direct | 22 22 3.1s
symmetric - column - substitution | 22 11 33 17.5s
order = NaturalOrder() | 2 1 3 12.6s
order = RandomOrder{TaskLocalRNG, Nothing}(TaskLocalRNG(), nothing) | 2 1 3 0.6s
order = LargestFirst() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :high2low, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :high2low, true}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :low2high, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :low2high, true}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:forward, :low2high, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:forward, :low2high, true}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:forward, :high2low, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:forward, :high2low, true}() | 2 1 3 0.5s
nonsymmetric - bidirectional - direct | 22 11 33 7.4s
order = NaturalOrder() | 2 1 3 0.6s
order = RandomOrder{TaskLocalRNG, Nothing}(TaskLocalRNG(), nothing) | 2 1 3 0.6s
order = LargestFirst() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :high2low, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :high2low, true}() | 2 1 3 0.8s
order = DynamicDegreeBasedOrder{:back, :low2high, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:back, :low2high, true}() | 2 1 3 0.8s
order = DynamicDegreeBasedOrder{:forward, :low2high, false}() | 2 1 3 0.5s
order = DynamicDegreeBasedOrder{:forward, :low2high, true}() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:forward, :high2low, false}() | 2 1 3 0.8s
order = DynamicDegreeBasedOrder{:forward, :high2low, true}() | 2 1 3 0.8s
nonsymmetric - bidirectional - substitution | 22 11 33 7.6s
order = NaturalOrder() | 2 1 3 0.7s
order = RandomOrder{TaskLocalRNG, Nothing}(TaskLocalRNG(), nothing) | 2 1 3 0.7s
order = LargestFirst() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:back, :high2low, false}() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:back, :high2low, true}() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:back, :low2high, false}() | 2 1 3 0.6s
order = DynamicDegreeBasedOrder{:back, :low2high, true}() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:forward, :low2high, false}() | 2 1 3 0.6s
order = DynamicDegreeBasedOrder{:forward, :low2high, true}() | 2 1 3 0.7s
order = DynamicDegreeBasedOrder{:forward, :high2low, false}() | 2 1 3 0.8s
order = DynamicDegreeBasedOrder{:forward, :high2low, true}() | 2 1 3 0.7s
Structured coloring | 8 8 0.7s
Sparse decompression | 528 528 50.2s
Structured decompression | 8 8 3.4s
Single precision | 242 242 59.8s
Allocations | 55 2 57 28.3s
ERROR: LoadError: Some tests did not pass: 64392 passed, 66 failed, 33 errored, 31 broken.
in expression starting at /home/amontois/SparseMatrixColorings.jl/test/runtests.jl:10
ERROR: Package SparseMatrixColorings errored during testing |
||||||||||||||||||||||||||||||||||||||||||
| # | ||||||||||||||||||||||||||||||||||||||||||
| # The C and Fortran interfaces compile SMC.jl into a standalone shared library. | ||||||||||||||||||||||||||||||||||||||||||
| # The trimming verifier rejects any call whose result type is not fully inferred, | ||||||||||||||||||||||||||||||||||||||||||
| # so `coloring` must return a concrete type. | ||||||||||||||||||||||||||||||||||||||||||
|
gdalle marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| using SparseArrays | ||||||||||||||||||||||||||||||||||||||||||
| using SparseMatrixColorings | ||||||||||||||||||||||||||||||||||||||||||
| using Test | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const STATIC_COMBOS = [ | ||||||||||||||||||||||||||||||||||||||||||
| (ColoringProblem{:nonsymmetric,:column}(), GreedyColoringAlgorithm{:direct}()), | ||||||||||||||||||||||||||||||||||||||||||
| (ColoringProblem{:nonsymmetric,:row}(), GreedyColoringAlgorithm{:direct}()), | ||||||||||||||||||||||||||||||||||||||||||
| (ColoringProblem{:symmetric,:column}(), GreedyColoringAlgorithm{:direct}()), | ||||||||||||||||||||||||||||||||||||||||||
| (ColoringProblem{:symmetric,:column}(), GreedyColoringAlgorithm{:substitution}()), | ||||||||||||||||||||||||||||||||||||||||||
| (ColoringProblem{:nonsymmetric,:bidirectional}(), GreedyColoringAlgorithm{:direct}()), | ||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||
| ColoringProblem{:nonsymmetric,:bidirectional}(), | ||||||||||||||||||||||||||||||||||||||||||
| GreedyColoringAlgorithm{:substitution}(), | ||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Symmetric, so that it is a valid input for every combination above. | ||||||||||||||||||||||||||||||||||||||||||
| const STATIC_MATRIX = sparse( | ||||||||||||||||||||||||||||||||||||||||||
| [1, 2, 1, 2, 3, 2, 3, 4, 3, 4], [1, 1, 2, 2, 2, 3, 3, 3, 4, 4], ones(10), 4, 4 | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @testset "coloring infers a concrete result type" begin | ||||||||||||||||||||||||||||||||||||||||||
| @testset "$(typeof(problem)) / $(typeof(algo))" for (problem, algo) in STATIC_COMBOS | ||||||||||||||||||||||||||||||||||||||||||
| for decompression_eltype in (Float32, Float64) | ||||||||||||||||||||||||||||||||||||||||||
| for symmetric_pattern in (false, true) | ||||||||||||||||||||||||||||||||||||||||||
| @test (@inferred( | ||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||
| (A, p, a, sp, R) -> | ||||||||||||||||||||||||||||||||||||||||||
| coloring(A, p, a; decompression_eltype=R, symmetric_pattern=sp) | ||||||||||||||||||||||||||||||||||||||||||
| )( | ||||||||||||||||||||||||||||||||||||||||||
| STATIC_MATRIX, | ||||||||||||||||||||||||||||||||||||||||||
| problem, | ||||||||||||||||||||||||||||||||||||||||||
| algo, | ||||||||||||||||||||||||||||||||||||||||||
| symmetric_pattern, | ||||||||||||||||||||||||||||||||||||||||||
| decompression_eltype, | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| )) isa AbstractColoringResult | ||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||
| end | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale behind the type annotation was to force specialization: https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing
Not sure how this change makes anything better in that regard, I would have thought it makes things worse?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too and it is the contrary that is happening, the compiler widen the type to
DataTypeinternally from what I unserstand if we keepType{R}and we don't have the inference in 3 cases.I am wondering if it is because the argument with the specialization is a keyword argument.