[ET Trac] [Einstein Toolkit] #1627: Merge rewrite branch of McLachlan

[Einstein Toolkit]

#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.

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/1627#comment:3>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit