Replace C++ code of SSIDS with Fortran - #542
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
==========================================
+ Coverage 26.93% 27.44% +0.51%
==========================================
Files 164 152 -12
Lines 29197 28151 -1046
Branches 8800 8419 -381
==========================================
- Hits 7863 7725 -138
+ Misses 18762 17958 -804
+ Partials 2572 2468 -104 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d5304b8 to
bc05027
Compare
|
Thanks, Alexis, that is really great. I will have a look as soon as I can ... I have managed to get a short-term temporary licence for the NAG library, and need to give this (the interface from the new QP solver to NAG) priority before the licence expires next week. I see that you also updated the makemaster file ... thank you!! I do actually wonder if we should rename the package so as not to confuse the world, particularly as Hussam is planning for the official ssids to evolve. And I need to write docs, and a Python interface. Do you use AI to help you here? |
|
Fantastic thanks @amontoison! We should also incorporate Hussam's adaptive front size fix in ralna/spral#271 as this results in a major performance improvement. Agreed we should probably change the name as the codebase has diverged so much at this point, unless @haldaas is interested in merging this upstream. |
|
Sorry to be an idiot, as usual I did a git checkout -b spike-ssids-cholesky-fortran which seems to have set things here, git branch --show-current but when I tried a git pull, I get here is no tracking information for the current branch. If you wish to set tracking information for this branch you can do so with: Should I follow this advice? |
|
Assuming that you did the checkout from the master branch that should work, as you created a new local branch off of the branch you were on previously. More standard practice is to do a |
|
On the makefile build under gfortran I see: Warning: Unused variable ‘adc’ ssids_factor.F90:720:35: nothing serious of course. But when I run the test program Exhaustive test of subroutine interface to ssids Error termination. Backtrace: in a call to GEMM. Superficially, it looks like this is trying to access a(m+1,*) of an a(m,n) matrix. So either the argument a(nelim+2, nelim+1) is wrong or the dimensions m-nelim-1, n-nelim-1 are? (perhaps a 0-based mistake)? Looking at the original C++, I think it should be a(nelim+1,nelin+1), but that could be wrong ... those pesky 0-based flattened index arrays strike again!! I wonder, going forward, if we shouldn't try to make everything here 1-based? It is not very natural in fortran to have nelim going from 0 to n-1 ... My only other comment here is that it would be nice to put back the C comments (albeit as fortran ones) as these do help explain what the code is trying to do. No hurry on this |
|
With the NAG compiler, I get Compiling types Error: types.F90, line 400: No data-edit descriptor for effective item |
|
Aren't "No data-edit descriptor for effective item" errors to do with incorrect format descriptors in write statements? Should hopefully be an easy fix. |
|
Yes, indeed, the error is the 200 FORMAT( '( A, I15 )' ) |
|
Same in the following subroutine 200 FORMAT( //, 3A, I2, A ) OK, fixed those, and removed the VOLATILE from |
|
Needless to say, nothing worked with ifx, when compiling ssids.F90 I get an error #5633: Internal compiler error: segmentation violation signal raised Please report this error along with the circumstances in which it occurred in a Software Problem Report. Note: File and line given may not be explicit cause of this error. with no clues to why it failed (deep in the libc stuff). But this is for oneapi/compiler/2024.2, I haven't got a more modern one, I'll see if I can get one ... OK, it compiles fine under 2026.1, but the test fails with the uninformative symbol lookup error: ../ssids/run_ssids: undefined symbol: __kmpc_alloc, version VERSION |
|
Finally, it compiles and runs with nvfortran |
You need to link with the OpenMP library of Intel: |
|
I tried linking against /opt/intel/oneapi/compiler/latest/lib/libiomp5.so Update: it was my fault, flang also has a libiomp5.so, and was further up my link chain. Changing the order sorted things out. |
|
I believe that to link against Intel OpenMP you need to use |
|
I haven't tried that, but I will ... that seems to work too. I still get the very annoying "OMP: Info #269: OMP_NESTED variable deprecated, please use OMP_MAX_ACTIVE_LEVELS instead" message, even though I have set OMP_MAX_ACTIVE_LEVELS ... but it seems that I actively have to remove OMP_NESTED to get rid of the message, doh! |
|
OK, I'm making progress. As I suspected, the call to gemm in ldlt_tpp_factor has an incorrect leading array index for both arguments a(.,), these The next issue is that ldlt_app_solve_diag fails for an edge case; IF (i+1 == n .OR. IEEE_IS_FINITE(d(2*i+3))) THEN ... the first clause is true, but fortran doesn't say in which order clauses should be checked, and the index for d in the 2nd is outside bounds. So a But I am now stuck on the alter test. In SUBROUTINE alter (the one in ssids.F90 not numerical_subtree.F90 ) calls subtree%alter() with a segment of the 2-D array that stores the B block (dimension 1 is for the diagonal and dimension 2 for the off diagonal). There is a single part to this array (akeep%nparts = 1), but the Anyway, I'll check all I have so far in, but will be off line for three days ... we have visitors AND my home ethernet is boosting from 32Mbps to 900, as total fibre has finally arrived in the village ... and I very much suspect that my internet provider will mess it up! |
|
I have pushed an update that I believe fixes the nagfor issue. This is in subroutine alter in ssids, in the call to subtree%alter. I think that this should be Given this, I am happy if you merge, but perhaps we should change ssids -> slblt everywhere first? |
|
If we are to take control of this package, really we should improve the test example as it is very primitive at the moment |
removed VOLATILE in definition of fcontrib in contrib_iface.F90
@nimgould @jfowkes
|
|
@nimgould I am working on the renaming Update: Nick can you do a final checking and merge the PR (squash + merge) please ? |
c52f632 to
fa8984a
Compare
|
Agreed. Tests for slblt pass here |
5 participants
Remove all the C++ and GPU related code of SSIDS.
I started that a long time ago...
It also removes the need of
hwloc.