#2077: include RNSID in ET ------------------------------------+--------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: review Priority: major | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: Hydro_RNSID ------------------------------------+---------------------------------------
Comment (by jonah.maxwell.miller@…):
Hi all, Roland Haas asked me to take a look this thorn. I've played with the examples and the documentation and poked my head in the code. It's very nice to have this tool packaged with the Toolkit. Thank you for that! I've included most of my comments inline, but I'll summarize here:
As far as I can tell, the code is clean and readable and looks like it should be maintainable. I have a few minor complaints---such as missing include guards in header files, but mostly this is nice and clean. My major complaint here is that it would be nice if there was better encapsulation of the original RNSID code. RNS.c and rnsid.c are basically identical except that rnsid.c contains the appropriate interpolation to a 3D grid at the very end. I'd rather there be a backend file which does all the numerics, then separate files for the CLI and ET frontends which simply read in parameter commands and output a solution or interpolate to the grid respectively.
The tests and examples are really complete and nice. I also really like that there is a single rpar file to generate a wide variety of initial data. That said, a little explanation at the top of each rpar file explaining how to use it and which scenarios it covers might be nice.
Other than some really minor things (like the inclusion of a nonexistent header file) the Cactus files (like schedule.ccl) all look good.
I'd really like to see more documentation. The current documentation is essentially a list of required literature. As with all scientific code, a complete description of the method, which already exists in the literature, would be very excessive. That said, I found it frustrating to dig through said literature to look up simple things---things like the ansatz for the differential rotation, the exact form of the ansatz metric, the compactification scheme, and the precise meaning of named variables such as s, mu, and axes_ratio. The documentation should provide a reference document for these details so that the educated yet forgetful reader doesn't need to dig through the literature to find them. At the very least the documentation should give equation numbers. Algorithmic details, such as the type of iteratiion method used (Newton-Raphson?) and the type of derivatives used (a particularly clever application of finite differences!) would also be nice.
There's a lot of commented-out code. This doesn't hurt maintainability, but it does hurt readability. Since the code is under version control, is there a reason this code can't be removed?
That about covers it. Overall, great to see this!