EinsteinExact arrangement should be added to the Einstein Toolkit

[Ian Hinder]

See discussion on the mailing list:

http://lists.einsteintoolkit.org/pipermail/users/2011-November/001596.html

The current tasks for this are:

1. Include arrangement documentation describing the code (Barry)

2. Review of the code by a non-author (Bruno and Peter)

3. Add to the ET thornlist

Keyword:

[Barry Wardell]

I have added arrangement documentation describing the code.

[Bruno Mundim]

The discussion about EinsteinExact points to the following command to clone its repository:

git clone --recursive git:github.com/barrywardell/EinsteinExact

I added the following lines to einsteintoolkit.th:

1. EinsteinExact arrangement !TARGET = $ARR !TYPE = git !URL = git:github.com/barrywardell/EinsteinExact !REPO_PATH= $2 !CHECKOUT = EinsteinExact/doc EinsteinExact/m EinsteinExact/tests EinsteinExact/EinsteinExact_Test EinsteinExact/GaugeWave EinsteinExact/KerrSchild EinsteinExact/Minkowski EinsteinExact/ModifiedSchwarzschildBL EinsteinExact/ShiftedGaugeWave EinsteinExact/Vaidya2

checkout and compilation seems all right. My question is: do we need the option --recursive here? it doesn't seem to affect cloning (but I don't have experience with that option). If it is really necessary, then probably GetComponents needs to understand that, doesn't it?

[Roland Haas]

--recursive pulls in the Metrics repository as a submodule which is needed to regenerate the Thorns. It is not needed otherwise.

[Bruno Mundim]

I have looked into the arrangement documentation and I thought it is really in good shape. I spot a few minor details there that would be good to have it fixed/commented soon:

1) Eq. 4 is missing the 3-metric in the time derivative term.

2) Eq. 5 seems to have two matrices describing rotations around the same axis. Is this intentional? I would expect to express the euler angles in terms of rotation matrices around each of the 3 axes.

3) Shouldn't we add/augment Roland's explanation about --recursive option in the documentation?

I am going to read more carefully the code base and test it this afternoon, however from what I have seen so far I strongly suggest to include this thorn in the einsteintoolkit.th.

Another issue: do a large fraction of maintainers have writing access to the repository?

[Barry Wardell]

Replying to [comment:3 rhaas]:

--recursive pulls in the Metrics repository as a submodule which is needed to regenerate the Thorns. It is not needed otherwise.

This was the case in the past, but I simplified things last week so that --recursive is no longer needed.

[Barry Wardell]

Replying to [comment:4 bmundim]:

I have looked into the arrangement documentation and I thought it is really in good shape. I spot a few minor details there that would be good to have it fixed/commented soon:

1) Eq. 4 is missing the 3-metric in the time derivative term.

2) Eq. 5 seems to have two matrices describing rotations around the same axis. Is this intentional? I would expect to express the euler angles in terms of rotation matrices around each of the 3 axes.

Thanks for pointing these out. I will look into and fix them.

3) Shouldn't we add/augment Roland's explanation about --recursive option in the documentation?

This is no longer relevant.

Another issue: do a large fraction of maintainers have writing access to the repository?

Currently myself, Ian Hinder and Erik Schnetter have commit access.

[Ian Hinder]

Barry, do you want to add one of the other maintainers as well, for example Frank? This would be to do last-minute emergency fixes etc.

[anonymous]

Replying to [comment:7 hinder]:

Barry, do you want to add one of the other maintainers as well, for example Frank? This would be to do last-minute emergency fixes etc.

Yes, I can give other people access if they send me their GitHub username.

[Bruno Mundim]

A few issues/questions for discussion:

1) When running the test suites I got a message that it is not runnable for EinsteinExact:

No runnable testsuites in arrangements: CactusBase CactusConnect CactusDoc CactusElliptic CactusIO CactusPUGH EinsteinBase EinsteinEOS EinsteinUtils ExternalLibraries TAT KrancNumericalTools EinsteinExact

However EinsteinExact does have a test suite at

EinsteinExact/EinsteinExact_Test/test/ShiftedGaugeWave.par

EinsteinExact_Test is correctly symbolically linked in the arrangements directory. Any ideas what could be going wrong here?

2) When I ran the ShiftedGaugeWave.par test case I found a bit suspicious that the values output were exactly one or zero for gxx, kxx, alp or betax. Was this intentional? Doesn't it make the test too trivial?

3) Note too that ADMBase::dtalp and ADMBase::dtbetax are not output at all despite enlisted to do so. I guess this is happening because initial_dtlapse and initial_dtshift default to "none". So we need to set those parameters in the test par file. In any case, I added the following parameters to the par file and I got an output with non trivial values:

ADMBase::initial_data = ShiftedGaugeWave ADMBase::initial_shift = ShiftedGaugeWave ADMBase::initial_lapse = ShiftedGaugeWave ADMBase::initial_dtshift = ShiftedGaugeWave ADMBase::initial_dtlapse = ShiftedGaugeWave

4) In the documentation it says that the code ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. Per example above, I don't think this is true.

5) There are a few instances of "CCTK_INT ierr = 0;" declarations/initializations that are not used afterwards. That could be taken out to avoid compiler warning complaints.

[Bruno Mundim]

3) Shouldn't we add/augment Roland's explanation about --recursive option in the documentation?

This is no longer relevant.

Then we should change the documentation to reflect this.

[Barry Wardell]

Thanks for the comments!

Replying to [comment:9 bmundim]:

1) When running the test suites I got a message that it is not runnable for EinsteinExact:

No runnable testsuites in arrangements: CactusBase CactusConnect CactusDoc CactusElliptic CactusIO CactusPUGH EinsteinBase EinsteinEOS EinsteinUtils ExternalLibraries TAT KrancNumericalTools EinsteinExact

However EinsteinExact does have a test suite at

EinsteinExact/EinsteinExact_Test/test/ShiftedGaugeWave.par

EinsteinExact_Test is correctly symbolically linked in the arrangements directory. Any ideas what could be going wrong here?

The tests require 1 process to run. Can you check that you are running with 1 MPI process?

2) When I ran the ShiftedGaugeWave.par test case I found a bit suspicious that the values output were exactly one or zero for gxx, kxx, alp or betax. Was this intentional? Doesn't it make the test too trivial?

3) Note too that ADMBase::dtalp and ADMBase::dtbetax are not output at all despite enlisted to do so. I guess this is happening because initial_dtlapse and initial_dtshift default to "none". So we need to set those parameters in the test par file. In any case, I added the following parameters to the par file and I got an output with non trivial values:

ADMBase::initial_data = ShiftedGaugeWave ADMBase::initial_shift = ShiftedGaugeWave ADMBase::initial_lapse = ShiftedGaugeWave ADMBase::initial_dtshift = ShiftedGaugeWave ADMBase::initial_dtlapse = ShiftedGaugeWave

These were the problems reported by Peter. Although I did fix them a few days ago (and I think Peter tested the fix), I somehow forgot to merge the fix into the master branch. I've now merged them into master so if you pull you should have something which resolves both of these problems.

4) In the documentation it says that the code ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. Per example above, I don't think this is true.

It ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. The setting of the initial data for all of the ADMBase variables is determined only by the initial_data parameter.

5) There are a few instances of "CCTK_INT ierr = 0;" declarations/initializations that are not used afterwards. That could be taken out to avoid compiler warning complaints.

I guess this must be a Kranc related thing? There was a separate discussion about fixing these things in Kranc, so once that is resolved the problem should also go away here.

[Barry Wardell]

Replying to [comment:10 bmundim]:

>>3) Shouldn't we add/augment Roland's explanation about --recursive option in the documentation? >This is no longer relevant.

Then we should change the documentation to reflect this.

Updated.

[Barry Wardell]

Replying to [comment:6 barry.wardell]:

Replying to [comment:4 bmundim]: > I have looked into the arrangement documentation and I thought it is really in good shape. I spot a few minor details there that would be good to have it fixed/commented soon: > > 1) Eq. 4 is missing the 3-metric in the time derivative term. > > 2) Eq. 5 seems to have two matrices describing rotations around the same axis. Is this intentional? I would expect to express the euler angles in terms of rotation matrices around each of the 3 axes.

Thanks for pointing these out. I will look into and fix them.

Point 1) has now been fixed. With regards to point 2), I think Eq. 5 is correct as it currently is, although I can understand how it might not be clear that that is the case. There are lots of different conventions with how to define the Euler angles. The particular version used by EinsteinExact is the one described at Mathworld [http://mathworld.wolfram.com/EulerAngles.html] and I believe is the same as is used by Exact.

[Ian Hinder]

The angle convention used in Exact is described in the comments of https://trac.einsteintoolkit.org/browser/EinsteinInitialData/Exact/trunk/src/boost.F77, and it mentions that MathWorld link. The intention was to replicate the conventions of Exact exactly.

[Bruno Mundim]

Ok, thanks for clarifying the convention! Should we add this link to the documentation?

[Bruno Mundim]

EinsteinExact_Test is correctly symbolically linked in the arrangements directory. Any ideas what could be going wrong here?

The tests require 1 process to run. Can you check that you are running with 1 MPI process?

I was using two MPI tasks! It works fine with 1 MPI process. Thanks!

4) In the documentation it says that the code ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. Per example above, I don't think this is true.

It ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. The setting of the initial data for all of the ADMBase variables is determined only by the initial_data parameter.

I am not sure I understood you here. The parameter file does need to set initial_lapse, etc, to ShiftedGaugeWave (otherwise it uses ADMBase defaults). So how come ShiftedGaugeWave is ignoring those parameters? It extends them indeed.

[Bruno Mundim]

Barry, do you want to add one of the other maintainers as well, for example Frank? This would be to do last-minute emergency fixes etc.

Yes, I can give other people access if they send me their GitHub username.

I created an account there with this username: bcmundim

Thanks!

[Frank Löffler]

Replying to [comment:7 hinder]:

Barry, do you want to add one of the other maintainers as well, for example Frank? This would be to do last-minute emergency fixes etc.

knarrff

[Barry Wardell]

Replying to [comment:7 hinder]:

Barry, do you want to add one of the other maintainers as well, for example Frank? This would be to do last-minute emergency fixes etc.

I have given Frank and Bruno access.

Replying to [comment:16 bmundim]:

>>4) In the documentation it says that the code ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. Per example above, I don't think this is true. >It ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. The setting of the initial data for all of the ADMBase variables is determined only by the initial_data parameter.

I am not sure I understood you here. The parameter file does need to set initial_lapse, etc, to ShiftedGaugeWave (otherwise it uses ADMBase defaults). So how come ShiftedGaugeWave is ignoring those parameters? It extends them indeed.

The current situation is that the thorns extend all the parameters: ADMBase::initial_data, ADMBase::initial_lapse, ADMBase::initial_shift, ADMBase::initial_dtlapse, ADMBase::initial_dtshift, ADMBase::evolution_method. There are two scheduled functions in each thorn, one which is conditionally scheduled depending on ADMBase::initial_data being set to the thorn name and the other which is conditionally scheduled depending on ADMBase::evolution_method being set to the thorn name. Both of these functions unconditionally set all of the ADMBase variables (lapse, shift, three-metric, extrinsic curvature, dtlapse and dtshift). Ideally this should be improved at some point so that the other ADMBase parameters are queried before setting the relevant variables.

[Barry Wardell]

Replying to [comment:15 bmundim]:

Ok, thanks for clarifying the convention! Should we add this link to the documentation?

I have added a reference in the documentation.

[Erik Schnetter]

It would suffice to check (in the paramcheck bin) whether these three parameters have the same value.

[Barry Wardell]

Replying to [comment:23 eschnett]:

It would suffice to check (in the paramcheck bin) whether these three parameters have the same value.

Is it currently possible to do this with Kranc? At one point I discussed having support for this kind of thing with Ian but as far as I recall nothing is implemented yet, is it?

[Bruno Mundim]

4) In the documentation it says that the code ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. Per example above, I don't think this is true.

It ignores initial_shift, initial_lapse, initial_dtshift and initial_dtlapse. The setting of the initial data for all of the ADMBase variables is determined only by the initial_data parameter.

I am not sure I understood you here. The parameter file does need to set initial_lapse, etc, to ShiftedGaugeWave (otherwise it uses ADMBase defaults). So how come ShiftedGaugeWave is ignoring those parameters? It extends them indeed.

The current situation is that the thorns extend all the parameters: ADMBase::initial_data, ADMBase::initial_lapse, ADMBase::initial_shift, ADMBase::initial_dtlapse, ADMBase::initial_dtshift, ADMBase::evolution_method. There are two scheduled functions in each thorn, one which is conditionally scheduled depending on ADMBase::initial_data being set to the thorn name and the other which is conditionally scheduled depending on ADMBase::evolution_method being set to the thorn name. Both of these functions unconditionally set all of the ADMBase variables (lapse, shift, three-metric, extrinsic curvature, dtlapse and dtshift). Ideally this should be improved at some point so that the other ADMBase parameters are queried before setting the relevant variables.

Ok, now I understand what you mean by "being ignored" by the code. However if we don't set, for example, ADMBase::initial_dtlapse in the parameter file then it is set to its ADMBase default, "none", and end up not allocating storage for that variable (as far as I understood). At the end we do need to set them all in the parameter file.

[Ian Hinder]

Barry and I did discuss this issue, and I think we came to the conclusion that the feature needed in Kranc would be to provide a list of conditions on parameters which must be satisfied, and a list of error messages to give if they were not. These checks would then be performed in ParamCheck. We would then insist for this thorn that all the initial data and evolution method parameters were set consistently.

[Barry Wardell]

Replying to [comment:25 bmundim]:

Ok, now I understand what you mean by "being ignored" by the code. However if we don't set, for example, ADMBase::initial_dtlapse in the parameter file then it is set to its ADMBase default, "none", and end up not allocating storage for that variable (as far as I understood). At the end we do need to set them all in the parameter file.

Good point. I have added some comments in the documentation pointing out that these parameters must be set in order to ensure storage is allocated.

[Barry Wardell]

Are there any more outstanding issues, or can this be added to the EinsteinToolkit thornlist now?

[Bruno Mundim]

In my opinion, it could be added to the thornlist now. Thanks!