aspect icon indicating copy to clipboard operation
aspect copied to clipboard

[WIP] Fastscape c++ integration with adapter

Open Minerallo opened this issue 7 months ago • 11 comments

Description

This PR is a follow-up to #5323.
Since development has moved to a new main branch, this new PR continues the work with a rebased and updated version of the FastScape coupling plugin.

The key update in this version is the use of the FastScapeAdapter interface by @benbovy and @MFraters , which provides a consistent deal.II mesh-based surface coupling for both Box and SphericalShell geometries.

The current objective is to restore functionality for the Box geometry, now using the adapter interface. SphericalShell support will be integrated next.


Checklist

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.

Minerallo avatar Jun 12 '25 17:06 Minerallo

@Minerallo I've been reading through some parts of the code and found that there are a number of member variables that are either not used, or only ever set. Specifically, these are:

  • [ ] fs_seed: never used
  • [ ] p: I could not find where this variable is read from the input file. The variable name is too short to tell whether it's used anywhere :-)
  • [ ] node_tolerance: I don't think this is used anywhere
  • [ ] precision: This variable is set, but I don't think it's ever used.
  • [ ] restart: Never used.
  • [ ] end_time: This variable is read from the input file, but then never used. If you plan on using it, my preference would still be to get the end time from the SimulatorAccess object, rather than duplicate it in this class.

It's possible that you have plans for some of these variables. But if you don't, would you mind pulling the newest version of the branch (including my commits) and remove these variables? Feel free to check off the checkboxes above once you've done that.

bangerth avatar Jun 13 '25 22:06 bangerth

@Minerallo I've been reading through some parts of the code and found that there are a number of member variables that are either not used, or only ever set. Specifically, these are:

  • [x] fs_seed: never used
  • [x] p: I could not find where this variable is read from the input file. The variable name is too short to tell whether it's used anywhere :-)
  • [x] node_tolerance: I don't think this is used anywhere
  • [x] precision: This variable is set, but I don't think it's ever used.
  • [x] restart: Never used.
  • [x] end_time: This variable is read from the input file, but then never used. If you plan on using it, my preference would still be to get the end time from the SimulatorAccess object, rather than duplicate it in this class. okay

It's possible that you have plans for some of these variables. But if you don't, would you mind pulling the newest version of the branch (including my commits) and remove these variables? Feel free to check off the checkboxes above once you've done that.

@bangerth I have removed all the variables for now, and will add them back if we need them later. p is the slop exponent for multidirectonal flow, this is computed internally in fastscapelib c++ now, right @benbovy ?

Minerallo avatar Jun 13 '25 23:06 Minerallo

There is an issue with n_grid_nodes, as just discussed.

For the box model, this variable isn't set at all. For the spherical model, you do

          n_grid_nodes = spherical_vertex_index_map.size();

but then (for both cases), in initialize(), you do

      n_grid_nodes = surface_mesh.n_active_cells();

which seems wrong simply based on the words: The left hand side indicates a number of nodes, the right hand side a number of cells.

Perhaps this kind of patch would seem right? I don't know what the rest of the code in this file does yet, so I'll let you figure out whether this makes sense:

--- a/source/mesh_deformation/fastscapecc.cc
+++ b/source/mesh_deformation/fastscapecc.cc
@@ -59,8 +59,6 @@ namespace aspect
       this->get_pcout() << "Geometry model type: " << typeid(geom_model).name() << std::endl;
 
       init_surface_mesh(geom_model);
-
-      n_grid_nodes = surface_mesh.n_active_cells();
     }
   
     template <int dim>
@@ -136,11 +134,13 @@ namespace aspect
                   }
               }
 
-          n_grid_nodes = spherical_vertex_index_map.size();
+          AssertDimension (spherical_vertex_index_map.size(), surface_mesh.n_used_vertices());
         }
       else
         AssertThrow(false, ExcMessage("FastScapecc plugin only supports Box or Spherical Shell geometries."));
 
+      n_grid_nodes = surface_mesh.n_used_vertices();
+
 
       // Having create the mesh, now set up the DoFHandlers and constraints objects
       surface_mesh_dof_handler.reinit(surface_mesh);

If that does make sense, please apply the patch for you locally.

bangerth avatar Jun 14 '25 00:06 bangerth

yes I would think that n_used_vertices() make more sense to use to set the vectors using the patch.

Quantity Value
surface_mesh.n_active_cells() 100
surface_mesh.n_used_vertices() 120

Minerallo avatar Jun 14 '25 02:06 Minerallo

which seems wrong simply based on the words: The left hand side indicates a number of nodes, the right hand side a number of cells.

FWIW, a Fastscapelib grid has no concept of a cell. Fastscapelib's algorithms for flow routing and erosion only need a grid (or a graph) of nodes connected to their direct neighbors. What those algorithms also need is an area surrounding each node, which may correspond to the area of a cell around the node (e.g., cell center) although the Fastscapelib doesn't need to know the exact geometry of the cell (which is expensive to store). There's some explanation here too: https://fastscapelib.readthedocs.io/en/latest/guide_grids.html#grid-representation

Something that we discussed during our last ASPECT/Fastscapelib coupling sprint in Potsdam: a Fastscapelib grid node may correspond to the center (centroid) of an ASPECT (deal.II) mesh cell, a node's neighbors may correspond to all adjacent cells and a node's "area" may correspond to the cell area. I'm not sure to remember how data fields in ASPECT are mapped to cell vertices (i.e., nodes) vs. cells in a deal.II mesh, but this is important to take into account when mapping ASPECT data onto a Fastscapelib grid.

benbovy avatar Jun 14 '25 03:06 benbovy

I see. I'll let the two of you figure out what is the appropriate approach. The current state does not seem useful since the variable is first set for the spherical shell grid in init_surface_mesh() (but not for the box geometry), and then immediately overwritten at the end of initialize(). I don't understand the code well enough to say how to fix this and will gladly let you figure this out :-)

bangerth avatar Jun 14 '25 03:06 bangerth

In #6204 the n_grid_nodes variable was set only once in initialize() after calling init_surface_mesh() for the appropriate geometry.

https://github.com/benbovy/aspect/blob/0a1a7f951bd878667d67b7fd0ed9996a91a58bde/source/mesh_deformation/fastscapecc.cc#L51-L101

I'm not sure why it has been changed since then.

I can try helping to figure out how to fix any issue related to n_grid_nodes (as it was implemented in #6204). However I might not be familiar enough with ASPECT to figure out what exactly is going on and what is needed, unfortunately.

benbovy avatar Jun 14 '25 03:06 benbovy

OK, that suggests that

n_grid_nodes = surface_mesh.n_active_cells();

is indeed the right piece of code, and the presence of

n_grid_nodes = spherical_vertex_index_map.size();

is wrong? @Minerallo I think I'll leave this to you then!

bangerth avatar Jun 14 '25 03:06 bangerth

If we set n_grid_nodes = surface_mesh.n_active_cells(); during initialization, it should work for both Box and SphericalShell geometries. That means I might be able to remove the line:

n_grid_nodes = spherical_vertex_index_map.size();

However, I think getting the Box geometry work, as simpler case will help me to better understand the logic.

Minerallo avatar Jun 14 '25 04:06 Minerallo

p is the slop exponent for multidirectonal flow, this is computed internally in fastscapelib c++ now, right @benbovy ?

@Minerallo in Fastscapelib C++ this is a parameter of the multi_flow_router flow operator. You can find some examples of usage here: https://fastscapelib.readthedocs.io/en/latest/guide_flow.html#flow-routing-strategies.

benbovy avatar Jun 14 '25 09:06 benbovy

@Minerallo FYI, I implemented the use of deal.II's send/receive functions. This should make it substantially easier to switch to the map.

bangerth avatar Jun 18 '25 22:06 bangerth