Hi,
is anybody using the [xyz]_offset parameters of TwoPunctures? I believe that their current implementation is broken and would like to fix it. However, I don't want to break anybodies parameter files in the process. So, if you do use those parameters, please let me know before I commit what I think fixes the problem.
Frank
On 23 Jun 2010, at 12:44, Frank Loeffler wrote:
Hi,
is anybody using the [xyz]_offset parameters of TwoPunctures? I believe that their current implementation is broken and would like to fix it. However, I don't want to break anybodies parameter files in the process. So, if you do use those parameters, please let me know before I commit what I think fixes the problem.
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? If so, in what way is it broken? We do use this parameter for unequal mass BBH simulations.
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)?
Frank
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.
On Thu, Jun 24, 2010 at 01:23:50AM -0400, Ian Hinder wrote:
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.
It seems that we agree that the later loop should also be removed now. If no one objects I will commit this later today.
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.
You are right. Didn't we think about a special place for large testsuites once?
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.
Newer versions of the subversion-tools contain a similar command: svn-bisect.
Frank
Hello Frank, all,
It seems that we agree that the later loop should also be removed now. If no one objects I will commit this later today.
I never saw the patch, did you post it to some other mailing list?
This issue also affects the matter sources (since TOVSolver uses Cactus' coordinate system where the BHs are at some arbitrary location and not TwoPunctures in which the punctures are at +/-par_b). Should already be fixed in the momentum_constraint branch (also fixes some other issues concerning matter sources).
Yours, Roland
On Thu, Jun 24, 2010 at 09:48:20AM -0400, Roland Haas wrote:
I never saw the patch, did you post it to some other mailing list?
No, I just described the loop, which is easy to find. Look for the offset parameter and where the coordinate GFs are changed.
This issue also affects the matter sources
That is how I noted it. I suddenly saw two matter blobs on the Cactus grid.
Should already be fixed in the momentum_constraint branch (also fixes some other issues concerning matter sources).
Do you think we could make that branch the default branch at some point? I don't like this diverging so much.
Frank
Hello all,
Should already be fixed in the momentum_constraint branch (also fixes some other issues concerning matter sources).
Do you think we could make that branch the default branch at some point? I don't like this diverging so much.
Works for me as long as some point is after the NRDA. It is somewhat tested ie. solving for a TOV at rest gives similar results than using TOVSolver alone. Solving for the momentum constraint still gives results that are not useful (terrible results due to taking second derivatives). This can be somewhat mitigated by having non-zero center_offset[0] and [1] (ie move the punctures off the x-axis).
Probably best if someone other than me looks at the code as well before we make it the default (vacuum is backward compatible, matter sources are not).
Yours, Roland
On Jun 24, 2010, at 1:23 , Ian Hinder wrote:
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?
No, the coordinate grid functions are only used by initial data. The PUGH and Carpet interpolators assume coordinates that can be expressed as "offset with delta", which is much more efficient than looking at grid functions.
Since one knows during wave extraction where the extraction sphere is located, the coordinate grid functions are probably also not used there (but this depends on the code). I assume that one would not interpolate coordinates (since this should be an identity), therefore errors in the coordinate grid functions should not matter there either.
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.
-erik
On 24 Jun 2010, at 09:24, Erik Schnetter wrote:
On Jun 24, 2010, at 1:23 , Ian Hinder wrote:
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?
No, the coordinate grid functions are only used by initial data. The PUGH and Carpet interpolators assume coordinates that can be expressed as "offset with delta", which is much more efficient than looking at grid functions.
Good!
Since one knows during wave extraction where the extraction sphere is located, the coordinate grid functions are probably also not used there (but this depends on the code). I assume that one would not interpolate coordinates (since this should be an identity), therefore errors in the coordinate grid functions should not matter there either.
Psi4 is a contraction of the Weyl tensor with a tetrad. The components of this tetrad are usually based on the coordinates, and the coordinate gridfunctions are typically used for this (at least in WeylScal4) since this is a pointwise operation at each point in the grid.
On 24 Jun 2010, at 09:34, Ian Hinder wrote:
On 24 Jun 2010, at 09:24, Erik Schnetter wrote:
On Jun 24, 2010, at 1:23 , Ian Hinder wrote:
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?
No, the coordinate grid functions are only used by initial data. The PUGH and Carpet interpolators assume coordinates that can be expressed as "offset with delta", which is much more efficient than looking at grid functions.
Good!
Since one knows during wave extraction where the extraction sphere is located, the coordinate grid functions are probably also not used there (but this depends on the code). I assume that one would not interpolate coordinates (since this should be an identity), therefore errors in the coordinate grid functions should not matter there either.
Psi4 is a contraction of the Weyl tensor with a tetrad. The components of this tetrad are usually based on the coordinates, and the coordinate gridfunctions are typically used for this (at least in WeylScal4) since this is a pointwise operation at each point in the grid.
Since the coordinate gridfunctions are reset on regridding, after the first regridding (probably within the first few M) Psi4 is corrected. This is visible as a discontinuity in the extracted Psi4 mode coinciding with the first regridding that causes the extraction level, or the level one finer, to change. See attached plot, where I extract at r = 10 M, and the dashed line at t = 1.08, represents the regridding. If the extraction level, or the one finer than it, is not ever regridded, then you will have to wait for the first recovery for the coordinates to be reset (the coordinates are not checkpointed).
users@lists.einsteintoolkit.org
-
Erik Schnetter -
Frank Loeffler -
Ian Hinder -
Roland Haas