On 23 Jun 2010, at 14:25, Frank Loeffler wrote:
On Wed, Jun 23, 2010 at 02:06:13PM -0400, Ian Hinder wrote:
I'm confused about what parameters you are talking about. In EinsteinInitialData/TwoPunctures/param.ccl, there is a parameter center_offset[i]. Is this the one you are referring to?
That is right.
We do use this parameter for unequal mass BBH simulations.
I suspected so.
If so, in what way is it broken?
It changes the grid functions x, y and z by that offset. You might be lucky and Carpet might overwrite that before the first time step though.
Do you still get the same results of you comment out the lines around 621 in TwoPunctures.c (the whole loop basically)?
I computed initial data before and after this change and wrote the BSSN conformal factor to HDF5. I then used h5diff to compare the resulting files. There were no differences apart from the build-id and simulation-id. However, the coordinates themselves *do* show differences, which I need to look into in more detail. The only thing I can think of in a BBH simulation which uses the coordinates gridfunctions is wave extraction, as they are often used to compute the tetrad. Are they also used by the interpolator? According to the schedule.ccl of CartGrid3D, it looks like the coordinates are reset on regridding. So it might be that the initial portion of the waveform before the first regridding is using incorrect coordinates. Luckily this portion of the waveform will almost certainly be before even the junk radiation in a typical simulation, so I think the impact of this should be minor. I will run a short test BBH simulation before and after this change and see what is affected.
This block of code was introduced in r70 of TwoPunctures, where it was accompanied by a similar block before the interpolation onto the cartesian grid which did the reverse transformation. I think the idea was to make a minimal change to the "complicated" twopunctures wrapper by modifying the coordinates temporarily and then changing them back again afterwards, which is fine. In r80, a patch was applied to "improve parallelisation by openmp" which changed to using offset coordinates directly in the twopunctures loop, and so removed the first loop which modified the coordinates, but didn't remove the later loop to put them back.
This probably could have been caught if the coordinates had been output in a test suite, but this is probably only knowable in hindsight. In principle it would have been caught by a wave extraction test, but only if that test used TwoPunctures initial data *and* the center_offset parameter. Maybe we should look into running couple of iterations of a full BBH simulation in a test suite? The only problem is memory usage. This can be solved with symmetries, but then that restricts the cases you can test.
PS: I tracked this down using "git svn", which allows you to treat an SVN repository as a git repository, so I could use the "git bisect" command. This command lets you easily perform a bisection search on the history saying "good" and "bad" for each commit it asks you about so you can determine which patch is the problem and why the problem happened.