Re::Solve SUNMatrix class#3
Conversation
… alias. The ReSolve tests now pass.
…ions to use sunmatrix generic tests
…ested)" This reverts commit d55cf1e.
…ity to choose output and indexing base for the print() function
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
A few minor, mainly nitpicking changes. It would be good to check why tests are failing.
| "Enable the SUNMATRIX_RESOLVE module (requires Re::Solve)" ON | ||
| DEPENDS_ON SUNDIALS_ENABLE_RESOLVE |
There was a problem hiding this comment.
This should be set to off for now.
There was a problem hiding this comment.
I believe DEPENDS_ON is a macro from cmake/macros/SundialsOption.cmake that will automatically set this to OFF if SUNDIALS_ENABLE_RESOLVE is not enabled. When I set it to OFF even with SUNDIALS_ENABLE_RESOLVE=ON the ReSolve tests are not created so I think this should be left as ON.
| -D CMAKE_CUDA_ARCHITECTURES="60" | ||
|
|
There was a problem hiding this comment.
Do we need to set an old CUDA architecture? Perhaps instructions should say something like "Optionally set your CUDA architecture, e.g. -D CMAKE_CUDA_ARCHITECTURES="80".
| Default: ``ON`` | ||
|
|
There was a problem hiding this comment.
Re::Solve support should be by default off.
There was a problem hiding this comment.
This default: ON is referring to the the compatibility checks. Looking at the documentation for the other TPL's these compatibility checks seems to always be default ON. Re::Solve support is default set to OFF above.
| - resolve~cuda~rocm arch=x86_64 %gcc@9.4.0 | ||
| config: |
There was a problem hiding this comment.
Did you verify that installing SUNDIALS with Spack works with this change?
| # if(SUNDIALS_MAGMA_BACKENDS MATCHES "CUDA") | ||
| # set_source_files_properties(sunmatrix_magmadense.cpp PROPERTIES LANGUAGE CUDA) | ||
| # set(_libs_needed sundials_nveccuda) | ||
| # elseif(SUNDIALS_MAGMA_BACKENDS MATCHES "HIP") | ||
| # set_source_files_properties(sunmatrix_magmadense.cpp PROPERTIES LANGUAGE CXX) | ||
| # set(_libs_needed sundials_nvechip hip::device) | ||
| # endif() |
There was a problem hiding this comment.
Why is this code commented out?
| /** | ||
| Utility function to print an array on the device for debugging | ||
| */ | ||
| // void SUNMatrix_ReSolve_Print_Array(SUNMatrix A) | ||
| // { | ||
| // if (RESOLVE_MEMSPACE(A) == ReSolve::memory::HOST) {return;} | ||
|
|
||
| // ReSolve::memory::MemorySpace memspace = RESOLVE_MEMSPACE(A); | ||
| // sunrealtype* d_data = SUNMatrix_ReSolve_Data(A, memspace); | ||
| // sunrealtype* h_data = new sunrealtype[SUNMatrix_ReSolve_NNZ(A)]; | ||
|
|
||
| // cudaMemcpy(h_data, d_data, | ||
| // SUNMatrix_ReSolve_NNZ(A) * sizeof(sunrealtype), | ||
| // cudaMemcpyDeviceToHost); | ||
|
|
||
| // for (int i = 0; i < SUNMatrix_ReSolve_NNZ(A); i++) | ||
| // { | ||
| // printf("data[%d] = %f\n", i, h_data[i]); | ||
| // } | ||
| // delete[] h_data; | ||
| // } |
There was a problem hiding this comment.
I suggest we remove commented out debugging code before merging.
| // Zero out the values of these arrays | ||
| for (i = 0; i < RESOLVE_NNZ(A); i++) | ||
| { | ||
| values[i] = ZERO; | ||
| index_values[i] = 0; | ||
| } | ||
|
|
||
| for (i = 0; i < RESOLVE_M(A); i++) | ||
| { | ||
| index_pointers[i] = ZERO; | ||
| } | ||
|
|
||
| (index_pointers)[RESOLVE_M(A)] = 0; |
There was a problem hiding this comment.
I would use memset functions here.
| SUNMatrix_ReSolve_SetUpdated(A, ReSolve::memory::HOST); | ||
|
|
||
| // Sync to device if necessary | ||
| if (RESOLVE_MEMSPACE(A) != ReSolve::memory::HOST) | ||
| { | ||
| SUNMatrix_ReSolve_SyncData(A, RESOLVE_MEMSPACE(A)); | ||
| } | ||
|
|
||
| return SUN_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Is this SUNMatrix public method? I think this is specific to Re::Solve matrix.
I think this function should be defined above
/* --------------------------------------------------------------------------
* Implementation of generic SUNMatrix operations.
* -------------------------------------------------------------------------- */
comment.
There was a problem hiding this comment.
Looking at this again there isn't a function definition here these are both calls by the generic SUNMatZero function to the Re::Solve specific functions SetUpdated and SyncData defined above the comment.
…ove the commented code
2 participants
Creation of the SUNMatrix_ReSolve class
resolve-developbranch notmain.CHANGELOG.mdanddocs/shared/RecentChanges.rstfiles. Notice that the former is a markdown file and the latter is reStructuredText, so the formatting is slightly different.