search

comp-physics / RBC3D

3D Spectral boundary integral solver for cell-scale blood flow
MIT License
7 stars 3 forks source link

Dependencies are outdated #6

Closed sbryngelson closed 3 weeks ago

sbryngelson commented 6 months ago

This codebase carries several very out of date dependencies. This makes it quite challenging to build and likely affects performance. Resolving this issue would substitute more up-to-date or modern/difference packages for such things. Below are some example packages.

suzanmanasreh commented 1 month ago

There is no hardcoded GMRES file. It's using a custom PETSc matrix shell operation in ModVelSolver.F90 and ModNoSlip.F90.

suzanmanasreh commented 1 month ago

I did manage to get petsc updated, and now common compiles with the new one, and I'm able to run simulations, but I wonder if it would be a good idea to be using an updated petsc with the old code because it doesn't provide any time cost/residual benefits, and it seems impossible to test if all the math is correct with the current library.

sbryngelson commented 1 month ago

We should always use newer library versions so we don't risk depreciation. Also, this would make it much easier for folks to do a module load petsc rather than build from scratch. You can test if the new library by making sure it gives the same results as the "original" code?

suzanmanasreh commented 1 month ago

It looks like it does give the same results as the old build from the simulations I've run so far. I was mainly worried because I had to make several changes to functions to get it to compile, and I don't fully understand those modules, but it should be fine.

suzanmanasreh commented 1 month ago

Oh also a custom petsc configure might still be necessary because the only reason I wanted to update it was to be able to run it in debug mode and see the source of crashes, but both the module load and custom configure work.

sbryngelson commented 1 month ago

Well we should make sure that the output of all our simulations match the previous version to within 14 significant digits or so. You can also isolate the input and output of the subroutines that call petsc routines (which would be a better way of doing it and more precise).

sbryngelson commented 1 month ago

Oh also a custom petsc configure might still be necessary because the only reason I wanted to update it was to be able to run it in debug mode and see the source of crashes, but both work.

@suzm10 but I would like it updated so we aren't relying on a special build of a very specific old version of petsc. If we need some custom process for using a debug build of petsc then that's an edge case we can support.

suzanmanasreh commented 1 month ago

Well we should make sure that the output of all our simulations match the previous version to within 14 significant digits or so. You can also isolate the input and output of the subroutines that call petsc routines (which would be a better way of doing it and more precise).

Okay, I'll do this for subroutines I changed. It's hard to use simulation data because of very large matrix sizes, but maybe it'll work with matrices I make up.

suzanmanasreh commented 1 month ago

Do you think building each case in /examples should be a CMake option in a top-level CMakeLists.txt or there should be a CMakeLists.txt in each case directory that someone can execute to build the case?

sbryngelson commented 1 month ago

there should only be one such cmake file

suzanmanasreh commented 1 month ago

Having a top-level one is going to make it so that every time someone adds a case, the CMakeLists.txt needs to extended. Unless they're all built at the same time which I don't really want to do because usually people work on cases individually.

sbryngelson commented 1 month ago

What do you think?

suzanmanasreh commented 1 month ago

I don't know. Maybe there's a smart way to do it that I'm missing. The fact that the cases have repeated file names makes it impossible to build all of them at the same time anyways. It'll be more obvious how to extend it if each case has its own CMakeLists.txt.

edit: solution i went for is have a build directory under top-level RBC3D directory. the top level cmake file includes subdirectories for common and custom targets for all the cases, so executing cmake .. in /build generates build/common and build/case directories that someone can run make common and make case on.

suzanmanasreh commented 4 weeks ago

CMake is working great for generating build files for 1 case at a time, but it throws lots of errors when I try to generate build files for all the example cases at the same time because they all have identical file names (initcond, tube). Would it be okay to rename files as part of this ticket? I think it makes more sense anyways.

CMake Error at CMakeLists.txt:135 (add_executable):
  add_executable cannot create target "initcond" because another target with
  the same name already exists.  The existing target is an executable created
  in source directory "/storage/home/hcoda1/4/smanasreh6/RBC3D".  See
  documentation for policy CMP0002 for more details.
suzanmanasreh commented 4 weeks ago

I'll just go ahead with the rename. I don't mind it.