Add interface for topography models. #814

Djneu · 2025-01-23T09:33:13Z

Here is adding the interface for having topography models and defining them in the .wb file. So far the model is named uniform, though I think this will be changed later on to something better fitted to the lowest feature topography model.

Next steps would be:

Make a function that allows world.cc to return the surface type when called from an outer program.
Combine uniform.cc with an actual topography model.

MFraters

I am on two minds about the naming of the class. Originally I thought just naming it none, to indicate that there is no topography. But uniform, with a default of zero would also work. Opinions?

include/world_builder/parameters.h

include/world_builder/topography/uniform.h

gassmoeller

Yes, the structure looks correct to me, but I have a number of suggestions for naming and you will have to update a bunch of comments.

include/world_builder/parameters.h

include/world_builder/topography/interface.h

gassmoeller · 2025-01-23T10:13:41Z

include/world_builder/topography/uniform.h

+
+        /**
+         * Whether to consider a reference or deformed surface.
+         */
+        const std::string  surface_type = "reference_surface";


while a string is flexible, it is also quite expensive to use. In addition it could contain arbitrary new values. I would suggest to introduce an enum type in the public part of this class (or even outside the class, but inside the Topography namespace. Something like:

enum SurfaceType { reference_surface, deformed_surface};

This also guarantees that only these two values can be used.

include/world_builder/topography/uniform.h

include/world_builder/topography/interface.h

gassmoeller · 2025-01-23T10:20:16Z

source/world_builder/topography/uniform.cc

+                        "Uniform gravity model. It returns the gravity vector in a Cartesian coordinate system at "
+                        "a given position, which has a constant magitude for the whole domain. The vector points "
+                        "down in cartesian coordinates and to the center of the sphere in spherical coordinates.");


nothing is parsed below so nothing needs to be declared here either?

Removed the declaration.

Djneu · 2025-01-23T12:38:09Z

Hi both, I think I've addressed all the previous comments. Next I think I'll need to add a case 7 to world.cc and make sure calling get_topography will return 0 and that I can also return the correct surface type with the get_surface_type function.

Apply suggestions from code review Co-authored-by: Rene Gassmoeller <[email protected]> Address comments. minor changes

anne-glerum · 2025-01-23T16:42:55Z

include/world_builder/parameters.h

+       * A pointers to the topography model. This variable is responsible for
+       * the topography model and has ownership over it. Therefore a unique
+       * pointer are used.


Just a random comment as I'm looking at PRs to see what everyone has been working on :)

Suggested change

* A pointers to the topography model. This variable is responsible for

* the topography model and has ownership over it. Therefore a unique

* pointer are used.

* A pointer to the topography model. This variable is responsible for

* the topography model and has ownership over it. Therefore a unique

* pointer is used.

anne-glerum · 2025-01-23T16:45:29Z

include/world_builder/topography/interface.h

+        SurfaceType surface_type;
+
+        /**
+         * declare and read in the world builder file into the parameters class


This comment doesn't seem right?

anne-glerum · 2025-01-23T16:49:31Z

include/world_builder/topography/interface.h

+        get_surface_type() = 0;
+
+        /**
+         * Returns a temperature based on the given position, depth in the model,


Suggested change

* Returns a temperature based on the given position, depth in the model,

* Returns a topography based on the given position, depth in the model,

anne-glerum · 2025-01-23T16:50:55Z

include/world_builder/topography/uniform.h

+         * declare and read in the world builder file into the topography class
+         */
+        static
+        void declare_entries(Parameters &prm, const std::string &parent_name = "");
+
+        /**
+         * declare and read in the world builder file into the topography class
+         */
+        void parse_entries(Parameters &prm) override final;
+
+        /**
+         * declare and read in the world builder file into the topography class


These comments are all the same, which makes them less informative. Maybe make them more specific to the functions?

gassmoeller

some comments from my previous review are not addressed yet. but generally closer to being ready.

gassmoeller · 2025-01-23T15:34:04Z

include/world_builder/parameters.h

+  namespace Topography
+  {
+    class Interface;
+  } // namespace GravityModel


gassmoeller · 2025-01-23T15:34:13Z

include/world_builder/parameters.h

      std::unique_ptr<WorldBuilder::GravityModel::Interface> gravity_model;

+      /**
+       * A pointers to the topography model. This variable is responsible for


pointers -> pointer

gassmoeller · 2025-01-23T15:34:28Z

include/world_builder/parameters.h

+      /**
+       * A pointers to the topography model. This variable is responsible for
+       * the topography model and has ownership over it. Therefore a unique
+       * pointer are used.


Suggested change

* pointer are used.

* pointer is used.

gassmoeller · 2025-01-23T15:34:35Z

include/world_builder/parameters.h

+       * A pointers to the topography model. This variable is responsible for
+       * the topography model and has ownership over it. Therefore a unique
+       * pointer are used.
+       * @see CoordinateSystem


what are you referring to here?

gassmoeller · 2025-01-23T15:36:22Z

include/world_builder/topography/interface.h

+          deformed_surface
+        };
+
+        SurfaceType surface_type;


Make this variable private. The enum type needs to be public, so that calling functions know that this type exists. But the variable itself (surface_type) should still be private as before so that no one accidentally changes this variable and all access has to go through get_surface_type.

Also add documentation what this variable means.

gassmoeller · 2025-01-23T15:37:50Z

include/world_builder/topography/uniform.h

+  {
+    /**
+     * This implements a minimum depth topography. The minimum depth topography
+     * finds the minimum depth of a feature and uses that for the topography.


Suggested change

* finds the minimum depth of a feature and uses that for the topography.

* finds the minimum depth of all active features at a location and uses that depth as the topography.

gassmoeller · 2025-01-23T16:50:44Z

source/world_builder/world.cc


-
-
  WorldBuilder::grains


why did you delete these lines?

MFraters · 2025-08-19T21:09:18Z

source/world_builder/topography/uniform.cc

+    double
+    Uniform::get_topography(const Point<3> &/*position*/,
+                            const Objects::NaturalCoordinate &/*position_in_natural_coordinates*/,
+                            const double /*depth*/) const


Do you expect the depth to be needed? Do you forsee that the returned topography would be different based on the depth?

MFraters · 2025-08-19T21:53:42Z

source/world_builder/topography/uniform.cc

+                            const Objects::NaturalCoordinate &/*position_in_natural_coordinates*/,
+                            const double /*depth*/) const
+    {
+      return 0;


uniform does not mean zero, it just means it is the same everywhere. could you make this variable?

MFraters

I started playing a little bit with it already and I am wondering why we choice the approach of making this one global plugin system for the whole model, instead of a plugin system per feature. I could for example imagine that for different features you might want to have different types of topography, for example on a continent use a digital elevation map and/or different elevation maps for different regions, while in the ocean use fpr exaple a isostatic equilibrium or a analytical solution from the half space cooling or plate model. Do you remember why we went for this approach?

MFraters reviewed Jan 23, 2025

View reviewed changes

include/world_builder/parameters.h Show resolved Hide resolved

include/world_builder/parameters.h Outdated Show resolved Hide resolved

include/world_builder/topography/uniform.h Outdated Show resolved Hide resolved

gassmoeller reviewed Jan 23, 2025

View reviewed changes

Djneu force-pushed the topo_model branch from cedfb17 to cefb474 Compare January 23, 2025 14:07

Djneu added 2 commits January 23, 2025 17:10

Add interface for topography models.

c1b8a9a

Apply suggestions from code review Co-authored-by: Rene Gassmoeller <[email protected]> Address comments. minor changes

Fix unused parameters

c3c6ee3

Djneu force-pushed the topo_model branch from 41cb55b to c3c6ee3 Compare January 23, 2025 16:10

anne-glerum reviewed Jan 23, 2025

View reviewed changes

gassmoeller reviewed Jan 23, 2025

View reviewed changes

MFraters reviewed Aug 19, 2025

View reviewed changes

MFraters mentioned this pull request Aug 24, 2025

[WIP] Add topograhpy infrastructure #848

Open

	* Returns a temperature based on the given position, depth in the model,
	* Returns a topography based on the given position, depth in the model,

	* finds the minimum depth of a feature and uses that for the topography.
	* finds the minimum depth of all active features at a location and uses that depth as the topography.

Add interface for topography models. #814

Are you sure you want to change the base?

Add interface for topography models. #814

Uh oh!

Conversation

Djneu commented Jan 23, 2025

Uh oh!

MFraters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gassmoeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Djneu commented Jan 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gassmoeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MFraters left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants