[ET Trac] #2823: Include TOVola: An enhanced TOV solver with Tabulated EOS support

#2823: Include TOVola: An enhanced TOV solver with Tabulated EOS support

Reporter: David Boyer Status: open Milestone: ET_2024_11 Version: Type: enhancement Priority: major Component: EinsteinToolkit thorn

Comment (by Roland Haas):

Comments looking at TOVola:​ ​

1. the documentation does not adhere to the rules in [http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-115000C1.8.4%5D(htt...) namely it does not correctly “namespace” the bibtex keys (important when building the ThornGuide which would see conflicting keys) 2. `STRING TOVola_ODE_method` in `param.ccl` should be `KEYWORD TOVola_ODE_method` 3. `0.0:* :: "Must be Positive"` is incorrect, unless non-negative is enough. Should use `(0.0:* :: "Must be positive"` as the range (assuming the docs [http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-187000D2.3.2%5D(htt...) still reflect the code and open intervals for double valued parameters have not been silently removed) 4. same as 3 for `TOVola_absolute_max_step` and `TOVola_absolute_min_step` and possibly others 5. `STEERABLE=always` probably makes no sense for a parameter used by an ID thorn, could be removed 6. test parameter files produce `*.par` files in output, violating [https://docs.einsteintoolkit.org/et-docs/Adding%5C_a%5C_test%5C_case%5D(http...) 7. `#include <cctk.h>` should be before any other includes (incl. system ones, since it brings in any DEFINES produced by ExternalLibraries). Should used `#include "cctk.h"` since `cctk.h`is not a a system provided include file ([https://www.gnu.org/software/c-intro-and-ref/manual/html%5C_node/include-Syn...) and [http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-92000C1.6.2%5D(http...). 8. `<gsl/gsl_odeiv2.h>` should use `"` same as in 7. 9. since it’s one file only, could make the various helper functions declared in `TOVola_ODE.h` all `static` (but is ok right now since properly namespaced) 10. `void TOVola_exception_handler(double x, double y[])` and other functions must not be _defined_ (contrasted to being _declared_ ) in header files since the compiler will create one copy of the code each time the header file is included leading to double defined symbols 11. not a bug: uses odd sort of indentation and placement of curly braces. Might want to consider using a “standard” formatting as shown in [https://bitbucket.org/cactuscode/cactus/src/master/.clang-format%5D(https://...) 12. `TOVola_TOV_Integrator_1` is scheduled in `local` mode (default), but only does global work, should be scheduled in global mode to not execute multiple times 13. `TOVola_TOV_Integrator_2` (which should be global, see 12) does error checks on what is essentially Cactus parameters, those should be done in a ParamCheck scheduled function so that they happen early on ([http://einsteintoolkit.org/referencemanual/ReferenceManual.html#x1-161000A2%...) 14. the whole set of functions `TOVola_TOV_Integrator_1, TOVola_TOV_Integrator_2, `and` TOVola_TOV_Allocate` seems strange. All of this could be done in a single function without having to escape out to the scheduler shouldn’t it? Using C++ one could use a `std::vector` and its `push_back` member to fill in the output array without having to compute its size first (speed does not matter here, safety and readability of the code does) 15. `TOVola_TOV_Allocate` does not handle failure from `malloc`, must be fixed 16. `TOVola_Raw_rSchw != NULL` is superfluous since `free` will accept a `NULL` pointer (and do nothing) [https://en.cppreference.com/w/c/memory/free%5D(https://en.cppreference.com/w...) 17. `#define velx (&vel[0*cctk_lsh[0]*cctk_lsh[1]*cctk_lsh[2]])` is incorrect and will fail of array padding is used (`cctk_lsh != cctk_ash`, [http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-98000C1.6.2%5D(http...). Don’t do this. use `CCTK_VECTGFINDEX3D` instead. Speed does not matter. If you do for some reason insist on not using `CCTK_VECTGFINDEX3D` for each access that at least do `#define vely &vel{CCTK_VECTGFINDEX3D(1, 0,0,0)] `but know that doing so will disable the range checks that `CCTK_GFINDEX3D` and related functions would do when Cactus is compiled in debug mode (`DEBUG=yes`). 18. in `( abs(x2-x1) == 1` I would think that for a properly coded bisection (why not use C’s `bsearch` [https://en.cppreference.com/w/c/algorithm/bsearch%5D(https://en.cppreference...) 19. `TOVola_TOV_Copy` misses `restrict` keywords on its pointer args making the compiler assume they may be aliasing the same memory location 20. assumes `int == CCTK_INT` which may not be true, I suggest to use one and use it consistently (or use `size_t` for array sizes and counters in array sizes) 21. `TOVola_interp_Drive` should use the `DECLARE_CCTK_ARGUMENTS_FOO` checked flavors of argument declarations and should declare its `READS` and `WRITES` in `schedule.ccl` ([http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-86000C1.6.2%5D(http...) 22. it does not checked the return code from `malloc`, this must be fixed 23. do not use `CCTK_INFO("TOVola Beginning Grid Placements...");` in a `local` scheduled function since it will output its info warning for each component creating very messy output. If users really need to know when a function runs they can run with `schedule_verbose` in Carpet. And in CarpetX `local` functions run multi-threaded making the output possibly overlap. Don’t do this. 24. `for(int i=0; i<cctk_lsh[0]; i++){` order of loops is sub-optimal, should be `k,j,i` to access memory in a contiguous manner. Should use `CCTK_LOOP3_ALL` to possibly make use of any (future) automated support for CarpetX 25. hard-codes the magic number `1e-30`. Don’t do this. Also note that for single precision `CCTK_REAL` (a compile time option of Cactus) this number may no longer differ from `0` if eg `TOVola_rho_baryon < 1e-8` eg in atmosphere region since single precision floats only go to `1e-38` or so. 26. why use `pow(1-2*TOVola_Mass/TOVola_rSchw_outside,0.5)` in line [https://github.com/dboyer7/TOVola/blob/66e190abe0b7d70b46cefe4704ca59557a89d...) but `sqrt` above. It _is_ a `sqrt` isn’t it and not a `pow(x,a)` with `a=0.5`? 27. `TOVola_interp_Driver` does no respect any of the various `initial_hydro` or `initial_data` or `initial_lapse` etc. parameters. This must be fixed. 28. `TOVola_interp_Driver` should set pressure as well since [http://einsteintoolkit.org/thornguide/EinsteinBase/HydroBase/documentation.h...) is not clear which sub-set of the primitives an ID thorn is expected to set (and [FIXME ]misses `w_lorentz`). 29. `CCTK_VWARN(CCTK_WARN_ABORT,` consider using `CCTK_ERROR` which is explicitly marked to the compiler as never returning 30. there seems to be a handful of superfluous include statements (eg `stdio.h` and `string.h` do not seem to be actually required to me) 31. instead of `{\tt TOVola}` I’d suggest using `\code{TOVola}` or `\textt{TOVola}` which is more LaTeX like and less plain-TeX like 32. why not use `equnarray` instead of the multiple successive `\begin{equation}\label{eqn:TOVdP}` like constructs?

‌

-- Ticket URL: https://bitbucket.org/einsteintoolkit/tickets/issues/2823/include-tovola-an-...