Merge rewrite branch of McLachlan

Comments (6)

1. 

   Erik Schnetter reporter

   • removed comment

   I have begun to document the changes at https://docs.einsteintoolkit.org/et-docs/Rewrite_McLachlan

   • 2015-01-01T21:11:54+00:00
2. 

   Erik Schnetter reporter

   • removed comment

   Add backward compatibility layer for parameters: Done.

   • 2015-01-06T13:40:42+00:00
3. 

   Ian Hinder

   • removed comment

   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.
   

   • 2015-01-26T09:56:44+00:00
4. 

   Erik Schnetter reporter

   • removed comment

   Merged.

   Answers to Ian's questions:

   • Performance benchmark results were posted to the mailing list. The upshot is that performance remains largely the same, except for McLachlan's built-in dissipation that is now much faster, probably slightly faster than thorn Dissipation.
   • Turning off dissipation: Yes, this is still a Kranc-time choice; this is necessary for performance. This the new performance is as good as or better than thorn Dissipation, I think this is now a non-issue.

   • Advection terms: The current code is not slowed down by the advection terms. I expect that other restructuring led to the same speed improvement that we gained from splitting out the advection terms previously.

   • ML_ prefix: I've kept this for backward compatibility.

   • FromBSSNCalc calculates everything that can be calculated from the BSSN state vector. This is not just the RHS, it also includes constraints, ADM variables, etc. Maybe "EverythingFromBSSNCalc" would be a good name. This change would be invisible to the user.
   • Renaming "phiW" to "cf": If so, then this needs to be done NOW. Please open a discussion on the mailing list.
   • Changing parameter names: I'd like to not do this for backward compatibility. Many parameter names don't change, and changing all of them is more disruptive than changing just a few gauge-related ones.
   • The helper thorn could be avoided. This seemed an orthogonal change, so I've omitted it for the time begin. We can look at it in a week or two, in case there's an unexpected problem with the rewrite.

   • 2015-07-24T07:21:08+00:00
5. 

   Erik Schnetter reporter

   • changed status to resolved
   • removed comment

   • 2015-08-14T14:03:12+00:00
6. 

   Roland Haas

   • edited description
   • changed status to closed

   • 2019-02-21T20:05:51+00:00
7. Log in to comment