Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #250 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 15 15
Lines 1824 1825 +1
=========================================
+ Hits 1824 1825 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark Results
|
|
@gdalle On the left it is with the modifications of this PR. |
|
My stance on Even if we actually wanted to use it, we would need tests beforehand to guarantee that the indices are indeed within the bounds (that's what base Julia does for dense linalg). For this sparse case, it would require iterating through all the row values to verify that they are in the correct interval, so the check has complexity Besides, speeding up coloring was useful for the paper to compare with ColPack, but in the real world, this kind of coloring is fast enough that no one will bat an eye anyway. I don't have many red lines, but unsafe memory accesses and aliasing is the biggest one. |
|
Enzyme is another beast and I totally agree with you that we need to put some limit sometimes. I agree that people can still do crazy things and break the sparsity pattern of a CSC matrix (but a very small %). Never do programming in low-level languages Guillaume, we don't have bound check in C or Fortran. |
|
The fact that SMC is coded in a high-level language and memory-safe by default is an asset, not a curse. I'll happily add a couple lines to the paper explaining that, but I don't want to compromise the safety of our software to get these last percentage points and match ColPack. If someone inadvertently puts a -1 in the rowvals while constructing their sparse matrix, they should get an error, not a REPL crash with random corruptions of memory. |
|
That being said, the present experiment is valuable and I thank you for running it! I just think it belongs in the paper as a comment, like you suggested, but not in the production-ready software. SciML and friends will already find a thousand ways to misuse our code, let's not give them one that is actually dangerous. |
|
I agree with you 🙂 |
|
Another alternative is to add a macro It makes us more comparable to ColPack because we remove the difference between the language Julia / C++ and we compare more the "real" modifications like what you did for the buckets. But I am also fine with just a comment about the bound checks in the paper. |
|
Let's stick with the comment then |
|
I close the PR. Conclusion: The compiler can't vectorize some operations because of bound checks. We want to keep them in SMC.jl to ensure that we can easily detect a mistake from the user. We will add a comment about that in the paper (C++ vs Julia). One short sentence is enough. |
|
Note that we haven't proven it's due to lack of vectorization. It may also be that the bounds checking itself takes significant time. This could be detected by profiling. |

I got a big difference on my ANL laptop, I don't know if it is related to the CPU or the
@inbounds.I will check with our benchmarks.