Modify

Opened 7 years ago

Last modified 5 years ago

#590 assigned defect

McLachlan should allow other thorns to set the gauge

Reported by: bmundim Owned by: Peter Diener
Priority: major Milestone:
Component: EinsteinToolkit thorn Version:
Keywords: Cc: users@…

Description

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.

Attachments (1)

Split-Gauges.patch (10.0 KB) - added by Peter Diener 6 years ago.
A patch that attempts to split out the gauge evolution from the main RHS

Download all attachments as: .zip

Change History (23)

comment:1 Changed 7 years ago by Roland Haas

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

comment:2 Changed 7 years ago by bmundim

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.

comment:3 Changed 6 years ago by Frank Löffler

Milestone: ET_2013_05

Making this a release goal, because this can (but should not) be really surprising for users.

comment:4 Changed 6 years ago by Ian Hinder

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.

comment:5 Changed 6 years ago by Erik Schnetter

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

comment:6 Changed 6 years ago by Peter Diener

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.

comment:7 Changed 6 years ago by Frank Löffler

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.

comment:8 in reply to:  7 Changed 6 years ago by Frank Löffler

Replying to 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.

comment:9 Changed 6 years ago by Frank Löffler

For the benefit of others: Peter is working on a patch.

comment:10 Changed 6 years ago by Frank Löffler

Owner: set to Peter Diener
Status: newassigned

Changed 6 years ago by Peter Diener

Attachment: Split-Gauges.patch added

A patch that attempts to split out the gauge evolution from the main RHS

comment:11 Changed 6 years ago by Peter Diener

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.

comment:12 Changed 6 years ago by Ian Hinder

Do the tests pass?

comment:13 Changed 6 years ago by Peter Diener

All the ML_BSSN_Test tests pass. I haven't run the complete ET testsuite set.

comment:14 Changed 6 years ago by Erik Schnetter

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

comment:15 Changed 6 years ago by Frank Löffler

Cc: users@… added

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.

comment:16 Changed 6 years ago by Peter Diener

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.

comment:17 Changed 6 years ago by Erik Schnetter

This does not seem to be advanced enough for this release.

comment:18 Changed 6 years ago by Frank Löffler

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

comment:19 Changed 6 years ago by Erik Schnetter

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.

comment:20 Changed 6 years ago by Ian Hinder

Milestone: ET_2013_05ET_2013_11

comment:21 Changed 5 years ago by Frank Löffler

Milestone: ET_2013_11ET_2014_05

comment:22 Changed 5 years ago by Ian Hinder

Milestone: ET_2014_05

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as assigned The owner will remain Peter Diener.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Peter Diener to the specified user.
The owner will be changed from Peter Diener to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.