[WIP] Fix Fastscape topography for periodic boundary by setting the values of ghost nodes earlier

[Kaili270]

Currently, if we use Fastscape with periodic boundaries for the front and back of the Fastscape domain, the front and back topography show abnormalities. See the topography at the front and back at the top of figure 1.

We fix this by setting the Stokes velocity on the ghost nodes that is sent to Fastscape before computing the direction of the periodicity from this velocity. This actually comes down to only moving code. See the results for the same setup in figure 2.

We will also test the left and right with periodic boundary conditions, but we have opened this pull request to start the discussion with @Djneu (and to practise making a pull request with @anne-glerum).

Before your first pull request:

• I have read the guidelines in our CONTRIBUTING.md document.

For all pull requests:

• I have followed the instructions for indenting my code.

For new features/models or changes of existing features:

• I have tested my new feature locally to ensure it is correct.
• I have created a testcase for the new feature/benchmark in the tests/ directory.
• I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

[gassmoeller]

This looks like it is a correct fix, but I am having trouble understanding what is happening in these lines, and why your change is a fix. This usually means the documentation needs to be improved. Could you extend the documentation as I indicated?

[gassmoeller]

Just to understand: The main difference of this change is that if we hit the else continue; statement below, this operation is done in any case, while previously it would only be done if we do not trigger that else, is this correct? Can you extend the comment in in line 1330 and in line 1335 to explain what you are doing in these lines, and why it is necessary?

E.g. I am not exactly sure what you are setting here. The comment says Set top ghost node but what are you setting it to? I suspect the velocity of the inner node which is actually the top boundary node of the ASPECT mesh, is this correct? And the number 2*fastscape_nx is also slightly mysterious, can you explain in the comment why we add or subtract that number?

[Djneu]

Hi both,
To add context, this specific issue arises because when using the periodic boundaries we check which direction the flow is going and set the ghost nodes on each side according to that. However, in 2D flow will be parallel to the periodic fastscape boundaries and there is no in/out flow, which would lead to the else continue; and the ghost nodes wouldn't be set to either side and this leads to odd topography that can't be eroded. By moving this code we default to one side and then can update to the other side if there is flow.

The way this is done was changed a bit before merging with the main branch, looking at it now we might not even need to change it based on the flow and always default to one side. From the comments on line 1216, the only difference for picking i side is to choose whether the outer nodes are set to e.g., "6 - 7 - 2" or "6 - 1 - 2", with 7 or 1 representing the ASPECT boundary nodes on each side. But if the nodes surrounding are always kept the same, I would assume 7 and 1 should also be identical.