#713: Implement "Conformal Covariant Z4" formulation in McLachlan ------------------------------------+--------------------------------------- Reporter: barry.wardell | Owner: Type: enhancement | Status: review Priority: major | Milestone: Component: EinsteinToolkit thorn | Version: Resolution: | Keywords: ------------------------------------+---------------------------------------
Comment (by barry.wardell):
Thanks for the patch. I have a few comments:
1. I'm not sure if it's wise to use a built-in function (Zeta) for a variable name. This might be fine but it seems possible that there could be unintended consequences at some point. For example, Attributes[Zeta]={Listable,NumericFunction}, which I could imagine causing different behavior in some circumstances. Some alternatives could be: [CapitalZeta], Z4, Zeta4.
2. Why did you remove the Theta terms from the dot[alpha] and dot[A] equations (and also from the convertToADMBase... calculations)?
3. Why did you remove the Zeta terms from R[la,lb] in the constraints calculation?
4. Would it be better to call the CCZ4Method parameter something more generic in case other formulations are added in the future?
4. This patch currently applies to the master branch. When committing it, I would suggest instead applying it to the CCZ4 branch and merging that with master. This would give a clear line of history leading back to Dana's original version which she tested quite thoroughly.
5. Do you know if this has any significant effect on performance? For example, could the extra code cause instruction cache issues?