#1627: Merge rewrite branch of McLachlan ------------------------------------+--------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: new Priority: major | Milestone: Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by hinder):
I have looked at the code on the rewrite branch and have a few comments/suggestions.
Performance:
* I would like to see performance benchmarks of the code on both branches before making such a drastic change. These should ideally include typical uses, e.g. finite difference orders 4, 6 and 8, and for 8, also with multipatch (Jacobians). Initially, just one standard Intel HPC machine would be enough to see if there were large regressions. A large amount of work went into optimising the master branch, and there is a risk that this work could be lost. * The old version allowed a version of the code to be generated which had kranc dissipation which could be efficiently omitted if apply_dissipation was "no". The new version requires this choice to be made at kranc time, or to have a version which always computes the dissipation. In the default, the dissipation terms are added unconditionally. These can be extremely expensive; the Dissipation thorn is much more efficient (due to the way the looping is done in Kranc, I think). I would prefer the default for krancDissipation to be 0, and people should be encouraged to use the Dissipation thorn, until dissipation is implemented more efficiently in McLachlan/Kranc. * As far as I can tell, the split of the advection terms into separate calculations has been lost in the rewrite. This was done for performance reasons after benchmarking, and made a big difference at the time. I expect it to still make a big difference.
Cosmetic:
* Why does ML_confac_bound have an ML prefix? Why not "phiW_bound"? * FromBSSNCalc seems to be strangely named. I would have expected something to do with "RHS" or "evolution". Probably "RHS". * There are three different conformal factors in common use: the original BSSN phi, chi as introduced by Brownsville, and W as introduced by Florida Atlantic. The current scheme of concatenating the names of the two implemented interpretations of the variable to make "phiW" from "phi" and "W" does not nicely scale to include chi. If renaming the variable anyway, I think I would prefer it to be called "cf" or "confac" or similar. * If changing parameter names anyway, we might want to follow the Cactus conventions and use lowercase names separated by underscores. This can be done by wrapping the string name in Parameter. e.g. Parameter["advect_lapse"]. If this makes the code less readable, we could set advectLapse as a Mathematica variable advectLapse = Parameter["advect_lapse"] and then use advectLapse in the equations. This functionality should work already. There may be some rough edges, but we only get to introduce new parameter names once, and we will have to live with them for a long time, so it might be worth getting parameter names that we like. * Do we still need to have a separate helper thorn? It seems to me that with the MergeFiles functionality in Kranc, this is not necessary. We would still provide a null helper thorn so that old parameter files work, but I think everything could go into ML_BSSN.