GEARRT: conservative flux exchanges

This MR does the same as !1411 (merged) did for Gizmo hydrodynamics: Change the RT interactions and flux exchanges to conservative ones.

Some additional attention was necessary for the RT sorts due to the sub-cycling.

This passes my tests, and I ran a couple of thousand (hundred) steps of EAGLE_12 (EAGLE_25) successfully (until something went wrong with hydro, which is a different problem.) Feel free to stress test this too if you so desire. There is a small catch though: The way to "chose" whether an RT scheme runs with symmetric MPI comms or not is a bit hidden. The MPI_SYMMETRIC_FORCE_INTERACTION macro is defined in src/part.h only when a flavor of Gizmo is selected as the hydrodynamics method. This remains unchanged: The RT will also only use symmetric MPI comms if a gizmo hydrodynamics flavor is selected.

As such, any RT scheme that runs on top of Gizmo hydrodynamics will also have symmetric flux exchanges. And vice versa: Any RT scheme running on top of SPH flavors will have SPH-like "force" loops.

@yuyttenhove , do you perhaps want to have a look at this MR too before the merge?

requested review from @bvandenbroucke

assigned to @matthieu

added 11 commits

• 017b93d7...c2a306a8 - 2 commits from branch master
• 767a5be5 - non-MPI version seems to work
• 089435f4 - MPI passes simple test without subcycling
• 8bee02bf - sort fix for MPI
• 914e9653 - apparently fixed MPI
• 2b1ef259 - dependency issue fix: need sort->rt_transport dependency for local cells
• 87bf00af - fixed sorting flag issues
• c8905d64 - wrongly addressing sort in non-super cell
• 71b89235 - revert engine.c to master.
• 017a89fc - cleanup before MR

Compare with previous version

Would be great to have @yuyttenhove's input indeed.

With the caveat that I'm by far not as familiar with the RT as Mladen, these changes do look good to me.

Going through the code I remembered something I have been thinking about considering the flux.dt, however. I think it might be set incorrectly for particles when their timestep has been limited. Consider for example a particle that was ending its timestep at ti=0 and ending its next timestep at ti=16, so with a dti=16. Suppose this particle gets woken up at ti=10 by a neighbour with a timestep dti=1. The particle now gets assigned the new timestep dti=4 and will end its timestep at ti=12 (instead of ti=16). Now at ti=12 the flux.dt of this particle just gets calculated from its current timebin (corresponding to a dti=4). I think that this is only guaranteed to be correct when exchanging fluxes with neighbours that have a timebin that is at most 1 higher then the new timebin of this particle. Considering for example a neighbour that also has a dti=16, we actually want to exchange a flux over a time interval corresponding to dti=12 and not dti=4 as will be the case currently.

Unless I have misunderstood how the timestep limiting happens exactly, what do you think?

I tried fixing this in the moving mesh scheme already, but now that I have been thinking more about it, I'm not sure it is entirely correct there either.

I'm also not entirely sure what the impact of this is on the RT subcycling, I think things are actually a bit less likely to go wrong, but there are certainly still edge cases where the wrong flux.dt will be used.

approved this merge request

Accepting.

Let's move the time-step limited discussion elsewhere.

mentioned in commit 445fb31d

merged

mentioned in issue #832