McLachlan should allow other thorns to set the gauge

Issue #590 open
Bruno Mundim created an issue

The parameters lapse_evolution_method and shift_evolution_method are usually set to ML_BSSN in McLachlan. However they are never checked in the code. McLachlan indeed seems to ignore their values and overwrite whatever the values of lapse or shift set elsewhere, preventing therefore other thorns from setting them differently.

Keyword:

Comments (22)

  1. Roland Haas
    • removed comment

    only those, you are saying it actually respects evolution_method? I am surprised :-)

  2. Bruno Mundim reporter
    • removed comment

    Not only those, but those were the ones I was concerned at the moment :) Indeed I should have said that McLachlan ignores any other values for the ADMBase extended parameters.

  3. Ian Hinder
    • removed comment

    I would like the gauge conditions to be separated out into separate calculations. This way, they can be controlled by logic in the scheduler, and we don't mix the mathematics of the evolution subsystems with the choice of which subsystems to combine. This is how the Penn State Kranc BSSN code works. Erik, do you agree? I believe everything is joined together at the moment for performance reasons. I suspect the performance benefit, if any, is slight, but of course we would test this.

  4. Erik Schnetter
    • removed comment

    The evolution and gauge equations are combined because one cannot combine arbitrary gauge and evolution conditions; stable formulations come as a package. Also, in the past, there were not many different gauge conditions available -- the choices were essentially whether A and B^i are evolved or not, or whether lapse and shift have advection terms.

    We can try splitting thing apart, and look at the performance impact.

    When splitting calculations apart, I worry about repetitive code. We already have some of this, e.g. the definition of the inverse metric, Christoffel symbols, and Ricci tensor; these are repeated for the evolution and the constraint code. We can try the following: - Have one "master" pseudo-calculation that defines all quantities, storing them in shorthands - Each actual calculation "imports" the master calculation, and then has access to these quantities - I believe Kranc already optimises away unused shorthands.

    For example, I am worried about repeating e.g. the definitions for dot[gamma-tilde], or about a scheduling dependencies if we want to read them back from grid functions before/after the advection terms have been applied, and doing so correctly when the RHS is evaluated either in MoL_RHS or at analysis.

  5. Peter Diener
    • removed comment

    I have been thinking about splitting the gauge conditions out for a while and agree that something has to be done. As the gauges are implemented right now, I have to go in and look at the Mathematica script in order to figure out how to set parameters to correspond (if possible) to old parameter files. It will also make it easier to experiment with new gauge conditions.There are some indications that the moving puncture gauge conditions are not optimal in all cases for matter evolutions.

  6. Frank Löffler
    • removed comment

    Actually - besides the good reasons already mentioned here, the gamma2 shift evolution of BSSN_MoL can be almost achieved with McLachlan with suitable parameters, except a factor the lapse in the source of the shift. Thus, adding that factor (obviously steered by parameter) would 'solve' the specific issue of the gamma2 shift I had - but not other issues like ignoring ADMBase parameters for example.

    Also, in case it comes up: adding an 'if' clause to the code is either not an issue or an already existing problem, because the shift evolution does already contain one - for the switch between the 1+log and the harmonic shifts.

  7. Frank Löffler
    • removed comment

    Replying to [comment:7 knarf]:

    Actually - besides the good reasons already mentioned here, the gamma2 shift evolution of BSSN_MoL can be almost achieved with McLachlan with suitable parameters,

    Would have been nice - but I overlooked one tiny detail which unfortunately makes a lot of a difference - so, no (almost)-gamma2 easily with the current version of McLachlan.

  8. Peter Diener
    • removed comment

    The above patch attempts to split out the lapse and shift evolution from the main RHS routines. It also implements simpler and much easier routines for Harmonic and 1+log lapse evolution and a routine that implements the gamma2 evolution scheme from BSSN_MoL. The lapse_evolution_method from ADMBase are extended by the values ML_BSSN_Harmonic and ML_BSSN_1+Log in addition to ML_BSSN and the corresponding routines are scheduled conditionally on those values. The choice ML_BSSN results in the same slicing as before. The shift_evolution_method parameter is similar extended by ML_BSSN_gamma2 in addition to the previously available ML_BSSN. The choice ML_BSSN results in the same shift as before. All gauge routines are scheduled after ML_BSSN_RHS1 and ML_BSSN_RHS2 but before ML_BSSN_Advect. The advection terms for the gauges are moved out from ML_BSSN_Advect and into the gauge evolution routines, since the advection terms should only be added if the gauge is evolved. I also moved dissipation for the lapse and shift variables out of ML_BSSN_Dissipation and into separate ML_BSSN_DissipationLapse and ML_BSSN_DissipationShift routines that are only scheduled if McLachlan is doing the gauge evolutions. Dissipation routines are scheduled after the advection routine.

    As far as I can see there is not a significant performance difference. Performance may even be a bit better.

  9. Erik Schnetter
    • removed comment

    John Baker also has changes to McLachlan; we should discuss how to merge them before applying this patch.

  10. Frank Löffler
    • removed comment
    • removed watchers

    Testing adding the ET users mailing list as CC to this ticket. But for that to work out in trac neatly would mean to respond in trac, not on the mailing list.

  11. Peter Diener
    • removed comment

    I took a quick look at the version of McLachlan provided in Ticket #963. As far as I can see the proposed changes there are orthogonal to this patch, i.e. they mainly change the RHS calculation, which I don't touch. John mentioned that he recently merged Jim's version with the release version. Maybe starting from that it would not be so difficult to produce a patch that can be applied to the development version.

  12. Frank Löffler
    • removed comment

    Peter, can you please comment? Are there any reasons not to include this so far?

  13. Erik Schnetter
    • removed comment

    This is a fairly invasive change to a very fundamental thorn. Such a change needs thorough testing.

    Peter says above that he didn't run the full set of test cases yet.

    This patch is also missing new test cases, both for the general case (everything handled by McLachlan) as well as the new cases (either lapse and/or shift handled by another mechanism). Presumably, one would not just record the new results, but also test that the results are correct, e.g. ensuring convergence with a gauge wave test case.

    What other thorn would make use of this functionality? I assume that those who asked for this functionality would be happy to test it with their additional gauge thorns (even if those are private thorns). If there is no one volunteering for such testing, then it would seem that we are developing functionality that is not actually currently needed.

  14. Ian Hinder
    • removed milestone
    • removed comment

    This is not going to make it for this release. I am removing the release milestone. If someone wants to make it happen for the next release, they can add a milestone for that. Doing this in the master branch seems pointless given the existence of the rewrite branch, however.

  15. Log in to comment