SimVascular / svFSIplus

This repository contains a line-by-line conversion of the Fortran svFSI solver to C++.
Other
6 stars 20 forks source link

Add interface linear solvers #204

Closed ktbolt closed 1 month ago

ktbolt commented 3 months ago

Current situation

This PR contains the new interface to numerical linear algebra packages (e.g. PETSc). See https://github.com/SimVascular/svFSIplus/issues/30.

It also includes updates to the tests/cases XML files to include a Linear_algebra section.

Release Notes

Solver input parameter XML files now require a Linear_algebra section.

mrp089 commented 3 months ago

@ktbolt, it looks like your branch is behind and can't be updated automatically. You'll have to update your main and git merge main in your feature branch. Before that, the GitHub actions won't run.

ktbolt commented 3 months ago

@mrp089 Ugh, of course, I should have merged before changing all of this XML files.

mrp089 commented 3 months ago

There's a build error:

/home/runner/work/svFSIplus/svFSIplus/Code/Source/svFSI/TrilinosLinearAlgebra.cpp: In member function ‘virtual void TrilinosLinearAlgebra::alloc(ComMod&, eqType&)’:
/home/runner/work/svFSIplus/svFSIplus/Code/Source/svFSI/TrilinosLinearAlgebra.cpp:89:24: error: ‘dof’ was not declared in this scope
   89 |     com_mod.Val.resize(dof*dof, com_mod.lhs.nnz);
      |                        ^~~
ktbolt commented 3 months ago

It turns out that the Trilinos interface trilinos_impl.cpp uses a bunch of global variables (e.g. dof) to store state. When not building with Trilinos dof was undefined.

The use of global variables will limit the usability of the Trilinos interface, can only use it in one equation.

mrp089 commented 3 months ago

Seg faults now in all tests:

[fv-az1379-175:09867] [ 0] /lib/x86_64-linux-gnu/libc.so.6( 0x43090)[0x7f8c5a53f090]
[fv-az1379-175:09867] [ 1] /lib/x86_64-linux-gnu/libgcc_s.so.1(_Unwind_Resume 0xd3)[0x7f8c5a6ff563]
[fv-az1379-175:09867] [ 2] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI( 0x1b399d)[0x5564e36f199d]
[fv-az1379-175:09867] [ 3] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(LinearAlgebraParameters::check_input_parameters() 0x464)[0x5564e37bab24]
[fv-az1379-175:09867] [ 4] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(LinearAlgebraParameters::set_values(tinyxml2::XMLElement*) 0x321)[0x5564e37e01f1]
[fv-az1379-175:09867] [ 5] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(LinearSolverParameters::set_values(tinyxml2::XMLElement*) 0x784)[0x5564e37e0ef4]
[fv-az1379-175:09867] [ 6] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(EquationParameters::set_values(tinyxml2::XMLElement*) 0x84f)[0x5564e37e208f]
[fv-az1379-175:09867] [ 7] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(Parameters::set_equation_values(tinyxml2::XMLElement*) 0x92)[0x5564e37e2482]
[fv-az1379-175:09867] [ 8] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(Parameters::read_xml(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) 0xe7)[0x5564e37e3847]
[fv-az1379-175:09867] [ 9] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(Simulation::read_parameters(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) 0x5f)[0x5564e382a4af]
[fv-az1379-175:09867] [10] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(read_files_ns::read_files(Simulation*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) 0x6ea)[0x5564e39d4b2a]
[fv-az1379-175:09867] [11] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(read_files(Simulation*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) 0x93)[0x5564e3905053]
[fv-az1379-175:09867] [12] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(main 0x153)[0x5564e37a7c13]
[fv-az1379-175:09867] [13] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main 0xf3)[0x7f8c5a520083]
[fv-az1379-175:09867] [14] /home/runner/work/svFSIplus/svFSIplus/tests/../build/svFSI-build/bin/svFSI(_start 0x2e)[0x5564e37ad87e]
[fv-az1379-175:09867] *** End of error message ***
MatteoSalvador commented 3 months ago

Thanks @ktbolt! In this PR, I think we should not only update the currently available, mostly fsils-related test cases with the new linear solver interface, but also make sure that both PETSc (with the newly supported preconditioners) and Trilinos match the same reference solutions by adding some additional tests. This would also increase code coverage and account for the new features in this PR. As @mrp089 suggested in #30, this can be done by building PETSc/Trilinos (or the relevant sub-parts) on the testing machines, or by using Docker images.

mrp089 commented 3 months ago

@MatteoSalvador, as we expected, some results are different now. However, I noticed that these are only cep. The linear and nonlinear solver tolerances are 1e-6, but we're checking Action_potential to 1e-10. Also, Max_iterations is one. I didn't change it in #175 because the cep tests were the only ones that weren't failing.

@MatteoSalvador, can you do the same setup as in #175? You can use the testing README as guidance (then we also see if that makes sense). Then @ktbolt can merge that into this branch and the tests should pass. Thank you!!

mrp089 commented 3 months ago

@ktbolt, there are a couple of tests that fail with Unknown XML element 'Preconditioner'. (@MatteoSalvador is looking after the cep cases):

test_cep.py .............F...F...F..                                     [ 26%]
test_cmm.py ......                                                       [ 33%]
test_fluid.py ......FFF............                                      [ 57%]
test_fsi.py FFF                                                          [ 60%]
test_heats.py .........                                                  [ 70%]
test_shell.py FF                                                         [ 73%]
test_stokes.py ...                                                       [ 76%]
test_struct.py ............                                              [ 89%]
test_ustruct.py FFF......                                                [100%]
mrp089 commented 3 months ago

@ktbolt, I agree with @MatteoSalvador. It looks like the Trilinos and PETSc interfaces are untested (Codecov will only run once the tests pass). We can talk about how to add these in our meeting today.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 44.58204% with 179 lines in your changes are missing coverage. Please review.

Project coverage is 62.11%. Comparing base (cf3422d) to head (d4bdd0b).

Files Patch % Lines
Code/Source/svFSI/TrilinosLinearAlgebra.cpp 0.00% 67 Missing :warning:
Code/Source/svFSI/PetscLinearAlgebra.cpp 0.00% 54 Missing :warning:
Code/Source/svFSI/Parameters.cpp 64.38% 26 Missing :warning:
Code/Source/svFSI/FsilsLinearAlgebra.cpp 69.38% 15 Missing :warning:
Code/Source/svFSI/LinearAlgebra.cpp 68.18% 7 Missing :warning:
Code/Source/svFSI/set_bc.cpp 40.00% 3 Missing :warning:
Code/Source/svFSI/LinearAlgebra.h 0.00% 2 Missing :warning:
Code/Source/svFSI/read_files.cpp 80.00% 2 Missing :warning:
Code/Source/svFSI/FsilsLinearAlgebra.h 0.00% 1 Missing :warning:
Code/Source/svFSI/l_elas.cpp 0.00% 1 Missing :warning:
... and 1 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #204 +/- ## ========================================== - Coverage 62.24% 62.11% -0.14% ========================================== Files 103 109 +6 Lines 26964 27171 +207 ========================================== + Hits 16785 16877 +92 - Misses 10179 10294 +115 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MatteoSalvador commented 3 months ago

@mrp089 I checked and all cep tests pass now, even on my local machine(s). Thanks @ktbolt for updating all the xml files coherently with the new linear algebra interface.

However, I see that there are still some conflicts for consts.h, eq_assem.cpp and ls.cpp. After resolving these, as we discussed yesterday, I would be inclined to put this PR on hold until both the Trilinos and PETSc interfaces are fully tested and checked for code coverage.

ktbolt commented 3 months ago

@MatteoSalvador What do you mean when you say that there are some conflicts for consts.h, eq_assem.cpp and ls.cpp? Do you mean there are merging conflicts?

MatteoSalvador commented 3 months ago

Yes, exactly, they should be all related to Aaron's PR, which has been approved yesterday.

mrp089 commented 3 months ago

@ktbolt, can you please add building PETSc and Trilinos to GitHub Actions and switch some test cases to use PETSc or Trilinos? That's the only way we will cover TrilinosLinearAlgera.cpp, PetscLinearAlgebra.cpp, and others. @MatteoSalvador and I are here to help.

ktbolt commented 3 months ago

@mrp089 I no longer have time allocated to work on svFSIplus, once I fix the current bug I'm working on then that's it.