Skip to content
Snippets Groups Projects

[WIP] Gizmo unification

Closed Matthieu Schaller requested to merge gizmo_unification into master

@nnrw56 and @bvandenbroucke I need your opinion on this.

The idea here is to merge back Bert's GIZMO branch with the rest of the code. At the moment, the master code is able to switch smoothly between SPH implementations that only require two loops over neighbours. This switch is done simply by changing the define in const.h. Note that in the longer run, that should be done in the configure script. All the tasks are now agnostic of the hydro variation being used and simply call the right hydrodynamic functions, which are brought it at compile time via the file hydro.h.

Now, in the case of GIZMO, there are three loops over neighbours, meaning that the changes are not just restricted to runner.c but that more tasks have to be added.

In order to accommodate for this and to be more generic, I propose this modification which gives a framework to construct simply any number of loops over neighbours and the corresponding number of ghost tasks.

To be even more generic, I have created two kind of loops the "gather" loops (SPH terminology) which correspond to DOPAIR1 in swift and the "symmetric" loops corresponding to DOPAIR2. Any particle-based implementation would need N gather loop followed by M symmetric loops (Gadget-2: N=M=1, Gizmo: N=2, M=1). In principle, there exist cases with (N=0, M=1) but that would be a very bad implementation. I know one variant of SPH with (N=3, M=1) but none with M>1. As a consequence there will N+M-1 ghost tasks.

The first changes are:

  • Create more generic names for the tasks.
  • Create an array of these tasks for each cell.
  • In 'runner_main()', implement a better way of switching between task subtypes.

Then comes the large bit.

In engine.c, we have have the function 'engine_maketask()' that does many things (ignoring gravity for now). It:

  1. creates the first loop over neighbours
  2. creates the ghosts
  3. duplicates the first loop and create the dependencies.

So, as a first step, I have created sub-functions that correspond to these operations (same for gravity which corresponds to more or less empty functions).

I have then made the last two functions more generic. They will now create a generic number of tasks and create the correct number of ghosts and links for the given hydro scheme. This comes at the cost of having some pre-processor magic, which is a bit ugly but users willing to add more loops should quickly understand what is going on.

Note that the gizmo code in that branch is still an empty shell. It compiles but won't do anything. Let's focus on the architecture for now. Similarly, I have not done the corresponding work for the MPI tasks but it should be a straight-forward extension.

So, my question to both of you: What do you think of this way of doing things ? I really would like to bring Bert and GIZMO back in the picture before attacking gravity such that we can make it as generic as possible.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Matthieu Schaller Added 72 commits:

    Added 72 commits:

  • Added 1 commit:

  • Please have look mostly at task.[ch], cell.[ch], engine.c and runner.c.

    Edited by Matthieu Schaller
  • @bvandenbroucke has the GIZMO branch ever been tested with MPI ? Looking at the code I feel like some MPI tasks are missing.

  • @matthieu no, it has not been tested with MPI, so I would not be surprised if things are missing.

    1. The changes you made look ok to me. The pre-processor directives however remind me of our version of Gadget, and I'm a bit afraid that the code could get pretty ugly if more schemes with different N and M are implemented... Not that I see a better way to do this at the moment.

    2. Where are the constants (N_NEIGHBOUR_LOOPS etc.) defined?

    3. Can you move the runner_doghost1 and runner_doghost2 functions to the hydro folder as well? It does not make much sense to keep them in runner.c, and that would put all gizmo specific stuff in the same folder. I would also advocate renaming hydro_prepare_force, hydro_end_force etc. hydro_prepare_loop2, hydro_end_loop2 etc., so that it is clear which functions link to which loop.

    4. What should happen with the Riemann solver? It is quite hydro specific, so maybe the main src folder is not the best place to put it. And it also does not make sense to compile it if you use SPH. On the other hand, I can imagine that non-gizmo methods could also use a Riemann solver (e.g. a moving mesh).

    5. I will have a look at porting the working gizmo branch to the generic version, and check if this is possible using the same kick and drift functions SPH uses (which I think is possible). And if you can tell me what tasks are missing for MPI, I will have a look at that as well.

    Edited by Matthieu Schaller
  • Thanks for the feedback !

    1. Yes, this is a bit of a worry. It is limited to a small set of functions so it is not too bad but I share your concern without having a better solution to offer thus far.

    2. They are defined in hydro.h

    3. That's the plan. Make everything more uniform across the board.

    4. They are already in the master branch in a sub-directory called riemann. As you mentioned they could be useful for other hydrodynamics scheme so keeping them separate from the hydro sub-directory is probably best.

    5. I think I have a version which works with MPI. I haven't pushed it to the repository yet as it still needs testing and also because I wanted to restrict the changes for you and Pedro to look at to a minimum first.

    Edited by Matthieu Schaller
  • @nnrw56 Do you have any feeling on how dirty the code would be if that gets merged in ?

  • This is a somewhat large change... I think it's probably best if you walk me through it, e.g. in a video call. We can talk about this tomorrow.

  • Ok, makes sense.

  • Would you have time to talk about it this week ? Otherwise it will have to be after Ostrava.

  • Matthieu Schaller Status changed to closed

    Status changed to closed

Please register or sign in to reply
Loading