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

Wed Oct 9 17:35:37 CDT 2024

#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](http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-115000C1.8.4) 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](http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-187000D2.3.2) 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\_a\_test\_case](https://docs.einsteintoolkit.org/et-docs/Adding_a_test_case)
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\_node/include-Syntax.html](https://www.gnu.org/software/c-intro-and-ref/manual/html_node/include-Syntax.html) and [http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-92000C1.6.2](http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-92000C1.6.2)\).
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](https://bitbucket.org/cactuscode/cactus/src/master/.clang-format)
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](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](https://en.cppreference.com/w/c/memory/free)
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](http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-98000C1.6.2)\). 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](https://en.cppreference.com/w/c/algorithm/bsearch)\)
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](http://einsteintoolkit.org/usersguide/UsersGuide.html#x1-86000C1.6.2)\)
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/66e190abe0b7d70b46cefe4704ca59557a89dc46/TOVola/src/TOVola\_Driver.c#L518C15-L518C59](https://github.com/dboyer7/TOVola/blob/66e190abe0b7d70b46cefe4704ca59557a89dc46/TOVola/src/TOVola_Driver.c#L518C15-L518C59) 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.html](http://einsteintoolkit.org/thornguide/EinsteinBase/HydroBase/documentation.html) 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-enhanced-tov-solver-with
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.einsteintoolkit.org/pipermail/trac/attachments/20241009/c9199ed6/attachment.htm>