Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fastscape c++ coupling [WIP] #5323

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Minerallo
Copy link

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

The existing coupling between ASPECT and FastScape, developed by Derek Neuharth and Anne Glerum, utilizes the Fortran version of FastScape. However, this Fortran version is no longer being actively developed. I want to couple ASPECT with the actively developed C++ version of FastScape (Fastscapelib), which will incorporate an unstructured grid and be parallelized. This will make ASPECT - FastScape working with large scale spherical models.
I have integrated the Fastscape C++ library to the contrib folder and added the paths to the Cmakelist. I re-use some of the fastscape fortran interface from PR #4396 to develop the initial FastScapecc (.cc, .h) which call the Stream Power law erosion function of fastscapelib.

I added a test case repository in tests repository with a output_screen but it is not working yet. It runs for few timesteps and crashes sending malloc errors.

Before your first pull request:

For all pull requests:

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.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some initial comments, mostly on the header file and variable names.

@@ -0,0 +1,311 @@
/*
Copyright (C) 2022 by the authors of the ASPECT code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2023

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not actually corrected :-)

Comment on lines +47 to +50
// /**
// * Destructor for FastScape.
// */
// ~FastScape() override;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented lines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, if you don't need the destructor at all, just remove the whole block here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

void parse_parameters (ParameterHandler &prm);

private:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unify the number of empty lines, we usually use 1 empty line if you want to separate lines logically. Also check the rest of the header file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay thanks corrected

Comment on lines +101 to +106
/**
* We want to check that every point within the FastScape mesh has been given a value.
* However, this is done only on processor 0, so this allows us to send an error later on
* if necessary.
*/
// mutable bool fastscape_mesh_filled = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented lines.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

/**
* How many levels FastScape should be refined above the maximum ASPECT surface resolution.
*/
unsigned int additional_refinement;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion

Suggested change
unsigned int additional_refinement;
unsigned int additional_refinement_levels;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

*/
unsigned int left;


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be consistent with the number of empty lines between parameters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corrected

* for the value to be transferred. This is only necessary if use_v is set to 0
* and the free surface is used to advect the surface with a normal projection.
*/
double precision;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be more self explanatory:

Suggested change
double precision;
double node_tolerance;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

Comment on lines 46 to 49
// Sub intervals are 3 less than points, if including the ghost nodes. Otherwise 1 less.
table_intervals[0] = nx ;
table_intervals[dim-1] = 1;
table_intervals[1] = ny;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment seems to contradict the code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes its corrected, I did not add ghost nodes yet and will see with Benoit how this could be directly done from fastscapelib.

Comment on lines 51 to 52
const GeometryModel::Box<dim> *geometry
= dynamic_cast<const GeometryModel::Box<dim>*> (&this->get_geometry_model());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I see you require a box geometry? This would be the place to add an Assert to ensure that only box geometries are used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is only box geometry for now I am adding back the assert. I will start to have a look how to integrate the use of the unstructured mesh (called trimesh) from fastscape for spherical models this week.

if (this->get_timestep_number() == 0)
return;

// std::vector<std::vector<double>> temporary_variables;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented code

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link

@benbovy benbovy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Minerallo I'm very glad to see your effort of coupling fastscapelib (C++) with aspect! Thanks for working in this.

I left a few comments. I hope it will be helpful.

Comment on lines +221 to +224
INCLUDE_DIRECTORIES("${CMAKE_SOURCE_DIR}/contrib/fastscape/include/")
INCLUDE_DIRECTORIES("${CMAKE_SOURCE_DIR}/contrib/xtensor/include/")
INCLUDE_DIRECTORIES("${CMAKE_SOURCE_DIR}/contrib/xtensor/include/xtensor/")
INCLUDE_DIRECTORIES("${CMAKE_SOURCE_DIR}/contrib/xtl/include/")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how you usually deal with external dependencies in aspect, but I think that using find_package(fastscapelib REQUIRED) then target_link_libraries(${ASPECT_TARGET} INTERFACE fastscapelib) would be a much better alternative. Fastscapelib, xtensor and xtl all provide CMake configuration files when installed so normally finding and linking against fastscapelib should also take care of transitive dependencies (xtensor and xtl).

Or you could use FetchContent or ExternalProject if you want to download those dependencies as part of the aspect's cmake configuration/build.

If for some reason you still want to vendor fastscapelib, xtensor and xtl sources in aspect, please include their license.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @benbovy, thanks for your comments, it's true that it will be a better way of doing it, I'll give it a try, it also makes more sense for the license problem. This was a quick way to get started and will have to be modified.

Comment on lines +122 to +127
// FastScape indexes from 1 to n, starting at X and Y = 0, and increases
// across the X row. At the end of the row, it jumps back to X = 0
// and up to the next X row in increasing Y direction. We track
// this to correctly place the variables later on.
// Nx*ys effectively tells us what row we are in
// and then indx tells us what position in that row.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this has been copied from #4396?

Beware that there are some major differences between fastscapelib-fortran and fastscapelib (C++). E.g., Fastscapelib-fortran uses a column-major layout (Fortran-style arrays) for representing 2D raster grid nodes on 1D arrays, while fastscapelib (C++) uses a row-major layout (C-style arrays).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it was thanks for pointing it out

Comment on lines +192 to +196
for (unsigned int i=0; i<array_size; ++i)
{
kf[i] = kff;
kd[i] = kdd;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only support spatially uniform values for these two parameters in aspect / fastscape coupling, you don't need to create kf and kd vector/array containers. Fastscapelib-fortran explicitly requires arrays but fastscapelib (C++) works with both scalar and array values.

Copy link
Author

@Minerallo Minerallo Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I didn't add it in this quick preliminary version of the interface, but these values may change spatially for example when using erosion depending of a prescribed wind direction (example in #4396).

Comment on lines +240 to +251
fastscapelib::raster_boundary_status bs(fastscapelib::node_status::fixed_value);

auto grid = fastscapelib::raster_grid::from_length({ static_cast<unsigned long>(nx), static_cast<unsigned long>(ny) }, { x_extent, y_extent}, bs);

// flow graph with single direction flow routing
fastscapelib::flow_graph<fastscapelib::raster_grid> flow_graph(
grid, { fastscapelib::single_flow_router(), fastscapelib::mst_sink_resolver() });
std::cout << "here it works 2"<<std::endl;

// Setup eroders
auto spl_eroder = fastscapelib::make_spl_eroder(flow_graph, 2e-4, 0.4, 1, 1e-5);
auto diffusion_eroder = fastscapelib::make_diffusion_adi_eroder(grid, 0.01);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to move the creation of the fastscapelib grid (as well as the flow_graph and eroder objects) to FastScapecc<dim>::initialize. This would be more efficient, especially if FastScapecc<dim>::compute_velocity_constraints_on_boundary is called multiple times and the size of the fastscapelib grid is fixed (as it seems to be the case looking at the values currently set in FastScapecc<dim>::initialize).

Those fastscapelib C++ classes don't provide default constructors, so if you want to have instances of those as members of FastScapecc you will probably need to wrap them in std::unique_ptr.

//xt::xarray<double> uplift_rate(vec_shape.shape(), 0);
// xt::xarray<double> uplift_rate = xt::adapt(uplift_rate_in_m_year,vec_shape.shape());

xt::xarray<double> uplift_rate(vec_shape.shape(), 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I haven't tested it yet, setting both the uplift rate and the (initial) elevation values to zero (vec_shape above) may work but may also make the simulation crash (undefined flow paths compute from the topographic surface).

(I assume you did this for quick testing here but your intent is to eventually set them from other aspect state variables?).

// apply channel erosion then hillslope diffusion
auto spl_erosion = spl_eroder.erode(uplifted_elevation, drainage_area, fastscape_timestep_in_years);
std::cout << "here it works 8e "<<std::endl;
auto diff_erosion = diffusion_eroder.erode(uplifted_elevation - spl_erosion, fastscape_timestep_in_years);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uplifted_elevation - spl_erosion returns an xtensor (lazy) expression rather than an xtensor container. Maybe this is the reason why the simulation crashes at this point (as I see in the "output_screen" logs)?

I'm actually surprised that it compiles and that it runs fine for a few iterations / time steps (xtensor's lazy expression system is powerful but it might be also tricky and confusing sometimes).

Could you try saving those elevation values in a temporary container before passing it to erode? I.e.,

xt::xarray<double> temp_elevation = uplifted_elevation - spl_erosion;
auto diff_erosion = diffusion_eroder.erode(temp_elevation, fastscape_timestep_in_years);

I should probably update the example in fastscapelib documentation too. Creating those temporary containers is not very nice here, better would be to update elevation inplace in each fastscapelib eroder erode() (I plan to add eventually add some overloads to support that).

std::vector<double> V(array_size);

// Initialize the variables that will be sent to FastScape.
std::vector<double> h(array_size, std::numeric_limits<double>::max());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you set initial default values to std::numeric_limits<double>::max() for the elevation container?

It doesn't seem like the safest way to initialize it regarding model stability (in case any element is left to its initial value later), but I might be missing something.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure that you even need to create h and h_old if the reason of those two variables was to interface it with fastscapelib-fortran.

Fastscapelib-fortran has its own array of elevation values (among other internal state variables defined in a context), which you normally cannot access directly using its API (you need a copy). With Fastscapelib C++ it is different: you can define your own (xtensor) array container and pass it to the C++ API.

So maybe all you need is an xt::xarray<double> elevation and copy its elements to/from other std::vector<double> variables defined in aspect?

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that I have 3 pending comments still. Let me flush them out.

{

/**
* A plugin that utilizes the landscape evolution code FastScape
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* A plugin that utilizes the landscape evolution code FastScape
* A plugin that utilizes the landscape evolution code FastScape++

*
*/
template<int dim>
class FastScapecc : public Interface<dim>, public SimulatorAccess<dim>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do the FastScape++ people write their software when they can't use anything other than letters? FastScapecc? Or FastScapepp? or FastScapeXX?

Comment on lines +47 to +50
// /**
// * Destructor for FastScape.
// */
// ~FastScape() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, if you don't need the destructor at all, just remove the whole block here.

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Minerallo There are a number of comments that suggest that you fixed something, but it doesn't show up in github. Did you forget to push these changes? Would you mind doing so?

@@ -0,0 +1,311 @@
/*
Copyright (C) 2022 by the authors of the ASPECT code.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not actually corrected :-)

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, my time's up. I'll see that I come back to this sometime soon, but perhaps these comments are already useful in some way!

@@ -0,0 +1,698 @@
#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is missing its customary header. Just take it from one of the other files, with the copyright year adjusted.

Comment on lines +3 to +13
#include <xtensor/xarray.hpp>
#include <xtensor/xmath.hpp>
#include <xtensor/xview.hpp>
#include <xtensor/xadapt.hpp>

#include <fastscapelib/flow/flow_graph.hpp>
#include <fastscapelib/flow/sink_resolver.hpp>
#include <fastscapelib/flow/flow_router.hpp>
#include <fastscapelib/grid/raster_grid.hpp>
#include <fastscapelib/eroders/diffusion_adi.hpp>
#include <fastscapelib/eroders/spl.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to have to figure out a way to disable compilation of this file when fastscape wasn't found during configuration. The way this is typically done is by #includeing global.h and then having a #ifdef ASPECT_WITH_FASTSCAPE_CC or some such.

#include <aspect/geometry_model/box.h>
#include <deal.II/numerics/vector_tools.h>
#include <aspect/postprocess/visualization.h>
#include <ctime>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this C++ header include down to where the other C++ headers are (specifically <algorithm>).

Comment on lines +35 to +41
FastScapecc<dim>::initialize ()
{
AssertThrow(Plugins::plugin_type_matches<const GeometryModel::Box<dim>>(this->get_geometry_model()),
ExcMessage("FastScape can only be run with a box geometry model."));

const GeometryModel::Box<dim> *geometry
= dynamic_cast<const GeometryModel::Box<dim>*> (&this->get_geometry_model());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will have to run ./contrib/utilities/indent to get a consistent indentation of the entire file. You'll like the end result, it will be much easier to read!

Comment on lines +48 to +61
// Get the deformation type names called for each boundary.
std::map<types::boundary_id, std::vector<std::string>> mesh_deformation_boundary_indicators_map
= this->get_mesh_deformation_handler().get_active_mesh_deformation_names();

// Loop over each mesh deformation boundary, and make sure FastScape is only called on the surface.
for (std::set<types::boundary_id>::const_iterator p = mesh_deformation_boundary_ids.begin();
p != mesh_deformation_boundary_ids.end(); ++p)
{
const std::vector<std::string> names = mesh_deformation_boundary_indicators_map[*p];
for (unsigned int i = 0; i < names.size(); ++i )
{
if (names[i] == "fastscape")
AssertThrow((*p == top_boundary),
ExcMessage("FastScape can only be called on the surface boundary."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Get the deformation type names called for each boundary.
std::map<types::boundary_id, std::vector<std::string>> mesh_deformation_boundary_indicators_map
= this->get_mesh_deformation_handler().get_active_mesh_deformation_names();
// Loop over each mesh deformation boundary, and make sure FastScape is only called on the surface.
for (std::set<types::boundary_id>::const_iterator p = mesh_deformation_boundary_ids.begin();
p != mesh_deformation_boundary_ids.end(); ++p)
{
const std::vector<std::string> names = mesh_deformation_boundary_indicators_map[*p];
for (unsigned int i = 0; i < names.size(); ++i )
{
if (names[i] == "fastscape")
AssertThrow((*p == top_boundary),
ExcMessage("FastScape can only be called on the surface boundary."));
// Get the deformation type names called for each boundary.
const std::map<types::boundary_id, std::vector<std::string>> mesh_deformation_boundary_indicators_map
= this->get_mesh_deformation_handler().get_active_mesh_deformation_names();
// Loop over each mesh deformation boundary, and make sure FastScape is only called on the surface.
for (const auto p : mesh_deformation_boundary_ids)
{
const std::vector<std::string> names = mesh_deformation_boundary_indicators_map[p];
for (unsigned int i = 0; i < names.size(); ++i )
{
if (names[i] == "fastscape")
AssertThrow((p == top_boundary),
ExcMessage("FastScape can only be called on the surface boundary."));

Comment on lines +90 to +120








//
// dx = 20000;
// dy = dx ;
// x_extent = 100e3;
// y_extent = 100e3;
// nx = 5 ;
// ny = 5 ;
// array_size = nx*ny;
//
// // Sub intervals are 3 less than points, if including the ghost nodes. Otherwise 1 less.
// table_intervals[0] = nx ;
// table_intervals[dim-1] = 1;
// table_intervals[1] = ny;

// const GeometryModel::Box<dim> *geometry
// = dynamic_cast<const GeometryModel::Box<dim>*> (&this->get_geometry_model());
//
// // The first entry represents the minimum coordinates of the model domain, the second the model extent.
// for (unsigned int i=0; i<dim; ++i)
// {
// grid_extent[i].first = geometry->get_origin()[i];
// grid_extent[i].second = geometry->get_extents()[i];
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have code that is no longer necessary, just remove it -- or document why you want to keep it.

Comment on lines +75 to +79
dx = (grid_extent[0].second)/( repetitions[0]);

// FastScape X extent, which is generally ASPECT's extent unless the ghost nodes are used,
// in which case 2 cells are added on either side.
x_extent = (grid_extent[0].second) ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dx = (grid_extent[0].second)/( repetitions[0]);
// FastScape X extent, which is generally ASPECT's extent unless the ghost nodes are used,
// in which case 2 cells are added on either side.
x_extent = (grid_extent[0].second) ;
dx = (grid_extent[0].second)/repetitions[0];
// FastScape X extent, which is generally ASPECT's extent unless the ghost nodes are used,
// in which case 2 cells are added on either side.
x_extent = grid_extent[0].second;

const types::boundary_id relevant_boundary = this->get_geometry_model().translate_symbolic_boundary_name_to_id ("top");
std::vector<std::vector<double>> temporary_variables(dim+2, std::vector<double>());

// Get a quadrature rule that exists only on the corners, and increase the refinement if specified.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Get a quadrature rule that exists only on the corners, and increase the refinement if specified.
// Define a quadrature rule that has points only on the vertices of
// a face, then subdivide it a number of times. This way, we can evaluate
// the solution at the surface nodes and points inbetween.

std::vector<std::vector<double>> temporary_variables(dim+2, std::vector<double>());

// Get a quadrature rule that exists only on the corners, and increase the refinement if specified.
const QIterated<dim-1> face_corners (QTrapez<1>(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give this variable a better name? Perhaps uniform_face_subdivision?

Comment on lines +152 to +160
for (const auto &cell : this->get_dof_handler().active_cell_iterators())
if (cell->is_locally_owned() && cell->at_boundary())
for (unsigned int face_no = 0; face_no < GeometryInfo<dim>::faces_per_cell; ++face_no)
if (cell->face(face_no)->at_boundary())
{
if ( cell->face(face_no)->boundary_id() != relevant_boundary)
continue;

std::vector<Tensor<1,dim>> vel(face_corners.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perhaps a more general comment:

The last line here is not very efficient. You are allocating memory on every face you encounter, but the array you allocate always has the same size. Two lines below you then fill it with some value. At the end of the code block, the memory is then released again.

A better scheme would have declared the variable just before the outer loop, because then the memory is allocated only once. Within the loop, you are then just filling the already allocated array, but the expensive memory allocation and deallocation only happens once in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants