FluidityProject / fluidity

Fluidity
http://fluidity-project.org
Other
365 stars 115 forks source link

Use PETSc fortran types instead of preprocessor macros #295

Closed gnikit closed 2 years ago

gnikit commented 3 years ago

I was wondering if there is a reason why the preprocessor macros for PETSc data types are used instead of the Fortran specific data types e.g. Mat :: should be type(tMat) ::? This mixing of preprocessor macros and Fortran code breaks syntax highlighting and linting with the fortran language server, which is a relatively minor problem but it ranks quite high in the annoyance scale.

If there is no reason to use the preprocessor macros, would you be open to replacing them with the appropriate PETSc Fortran types?

stephankramer commented 3 years ago

There's a long history in petsc of different ways of using their fortran interface (include files vs. fortran modules, f77 vs f90, derived type vs. mere wrapped integers) that existed side-by-side, with at some point 4 different supported ways, not all offering full functionality - see this horror show. Using the Mat preprocessor directives is what is currently documented as the recommended way (at least in the manual, https://www.mcs.anl.gov/petsc/petsc-current/docs/manualpages/Sys/UsingFortran.html mentions both)

So I'm inclined to just stick with what seems to be the more documented public interface. Also, more generally I'm a little wary of code changes for pure aesthetic reasons (refactoring just for the sake of it, indentation, etc.), in particular in a large monolithic code like fluidity. Code that has stood the test of time (tested in the CI sense, but also actually used by real users) should be preferred over newly written code. There's always a lot more unexpected consequences of seemingly trivial changes than anticipated, bugs (re)introduced etc. - and, it creates work for other people, potential for conflicts in branches, and more generally messing up git history...

gnikit commented 3 years ago

Thanks, that is a completely understandable response. We just haven't figured out a way to handle the mixing of preprocessor macros with normal fortran code, neither in the syntax highlighting side of things, nor with the linting so this leads to VSCode failing in all sort of unpredictable ways. Having said that Fluidity uses a very small fraction of PETSc variables when comparing it to something like FETCH, so realistically that will not be an issue for Fluidity. As for the manual, I suspect that it might be slightly outdated.

One last thing that I wanted to ask is whether would you consider it a useful change to have explicit interfaces for PETSc and all other external dependencies (similar to BLAS and LAPACK), in an attempt to catch potential errors during compile time?