Add Apparent Horizon Finder

[tamaraevst]

Hi all,

I have tested the AH finder on Cosma7, everything ran smoothly and all the AH data was produced as expected. The testing included:

1. Making all the tests and checking if there are any compilations errors
2. Running Kerr BH example
3. Running Binary BH example

To note: I have removed the SingleBH example here that was in the original feature/ah_finder.

In relation to the BBH run, I have slightly experimented with the courant factor, I found that I could increase the courant factor to 0.4 to stably evolve the binary, whilst gaining significant speed-up.

[mirenradia]

@tamaraevst, I'm still reviewing the files (unsurprisingly, it is taking a while). In the mean time, would you be able to resolve the merge conflicts by merging the main branch into this one?

[tamaraevst]

@tamaraevst, I'm still reviewing the files (unsurprisingly, it is taking a while). In the mean time, would you be able to resolve the merge conflicts by merging the main branch into this one?

Hi, @mirenradia, thank you and yes! I have just pushed the latest commit with resolved conflicts. Please have a look.

[mirenradia]

Hi, @mirenradia, thank you and yes! I have just pushed the latest commit with resolved conflicts. Please have a look.

Hmm, I'm not sure this has worked as GitHub is still displaying conflicts. Furthermore, the tests that were previously passing have started failing and I think this is due to the removal of the change

-DIM  = 3
+DIM ?= 3

in the Make.defs.local files used by the tests (since there is a 2D Apparent Horizon Finder test which overrides with DIM = 2).

I'm curious as to what you did?

I think the best approach would be to do

1. Revert your last commit (I will do this).
2. Note that since I will delete your latest commit, your local branch will be ahead of the remote tracking branch so it will probably be easiest to delete it, fetch from GitHub and check it out again using the commands

   git checkout main
   git branch -d testing/ah_finder
   git fetch
   git checkout testing/ah_finder

3. Make sure your local main branch is up to date with GitHub using the commands

   git checkout main
   git pull
   git checkout testing/ah_finder

4. Attempt to merge main into this branch using the command

   git merge main

5. Manually resolve the conflicts and complete the merge (git status should tell you want to do).

Let me know if you need any help :slightly_smiling_face:

[tamaraevst]

Hmm, I did exactly that, but I think the removal of DIM = 3 was due to it auto-merging Make.defs.local files. Let me try this again, and I will go through file by file which were merged automatically.

[mirenradia]

Hmm, I did exactly that, but I think the removal of DIM = 3 was due to it auto-merging Make.defs.local files. Let me try this again, and I will go through file by file which were merged automatically.

Weird, OK. I'll have a go then.

[tamaraevst]

Firstly, thanks for taking this on @tamaraevst . We really should have tried to merge this earlier when Tiago was still around. and your efforts are hugely appreciated.

Secondly, please don't be alarmed at the large number of changes/comments. They fall into these broad categories:

1. Cleaning up the dimension-dependent code by switching the macros used from Chombo's SPACE.H file to reduce code duplication and improve readability.
2. Replacing the SmallDataIO::read() function with the SmallDataIOReader class I developed and can be found here. As they currently are, some of the changes will break my SmallDataIO postprocessing tools hence why this is necessary.
3. Minor changes/comments which should be straightforward to resolve.
4. Things we might want to think about/discuss with others.

The vast majority of requested changes fall into category 1. and there are not many of category 4. I can fix the changes in category 2.

I suggest we proceed as follows:

1. I update the GitHub actions to build with PETSc and the AHFinder so that further changes are tested automatically CI. I will try to do this ASAP.
2. Once that is done, you can start actioning fixes in categories 1 and 3 (hopefully it should be reasonably obvious which kinds of thing fall into 3). Bear in mind that I haven't tested my suggested changes so they might break things/not compile. I think we can assume they are fine if all the tests pass (including the 2D and 3D tests).
3. Probably after that, I will resolve category 2 but I imagine this will take slightly more time.
4. Whilst steps 2 and 3 are going on, we can discuss anything in category 4 (@KAClough, your input would be appreciated here).

At some point, we will need to clang-format all of the code but this is not urgent.

I can re-review after the above steps but it would be appreciated if @KAClough can review any changes I make.

Hi Miren, sounds good to me, I will start working through points numbered 1) and 3).

[mirenradia]

Ok, I have now refactored the GitHub actions CI so that they run the Apparent Horizon tests.

As part of this refactoring, for each compiler (GCC, Clang, Intel Classic), I have further set up a "matrix" of jobs which tests different configurations (e.g. MPI = TRUE/FALSE, DIM = 2/3, OPT = TRUE/FALSE, ...) and compiler versions. For the Intel (Classic) compiler, I have stuck with just the latest version of the compiler. Since there are no Ubuntu packages built with Intel MPI, we cannot test the AHFinder (since this requires PETSc to be built with the same MPI implementation) with this stack.

[mirenradia]

@KAClough, would you be able to confirm you are happy with the changes I have made to the automated GitHub actions (in the last 10 commits)?

[mirenradia]

Following my recent commits, I have now actioned step 3 (i.e. replacing SmallDataIO::read with SmallDataIOReader).

Given these changes mostly affect restarting from a checkpoint, it would be great if @KAClough tests this carefully.

I have also taken the liberty of Clang-formatting the code.

[KAClough]

Ok, back to you for the changes @tamaraevst. Note that I have added documentation in the wiki here.

[KAClough]

Ok, for me this all works now!

I will not merge until Miren confirms he is ok too, but I would say that it is good enough to go public (as Voltaire said, le mieux est l'ennemi du bien.)

Once merged we should file an issue to say that changing the number of points on restart has been forbidden because it is broken, and this should be fixed by the next person that needs that feature.

If Miren does have time I think adding a check that the number of petsc ranks <= total mph ranks would be the most useful improvement at this stage, as it currently seems not to be checked, and leads to a very nasty late time error that is hard to debug.

[mirenradia]

Subject to the reversion of the AH_activated parameter name change, I think I am happy for this to be merged.