CLN: refactor C++/pybind11 single variables to structs #1294

jcrivenaes · 2025-01-16T14:35:14Z

Resolves #1295

Before we transfer more code from C/swig or make too much new code in the C++/pybind11 domain, a refactoring to structs which provide simple "shadow classes" for most important attributes from e.g RegularSurface() and Grid() is done.

It should give an easier to read and a more maintainable code, although I would think that further improvements are possible (perhaps doing it step-wise). In general C++ struct what applied instead of class at this stage, but it should be fairly simple to convert the structs to classes in a later stage, if needed.

Although many files are changed (due to massive dependencies), the PR shall not change any current functionality for the end user. In terms of CPU speed, a coarse comparison indicates no significant changes in on the overall test suite.

codecov-commenter · 2025-01-17T07:09:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.12%. Comparing base (0375fed) to head (7d0d54e).
Report is 66 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1294      +/-   ##
==========================================
+ Coverage   80.02%   81.12%   +1.09%     
==========================================
  Files          98       94       -4     
  Lines       13680    12481    -1199     
  Branches     2203     1882     -321     
==========================================
- Hits        10948    10125     -823     
+ Misses       1999     1692     -307     
+ Partials      733      664      -69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jcrivenaes · 2025-01-23T07:58:15Z

.clang-format

@@ -41,7 +41,7 @@ BreakBeforeBraces: Mozilla
 BreakBeforeInheritanceComma: true
 BreakBeforeTernaryOperators: true
 BreakConstructorInitializersBeforeComma: false
-BreakConstructorInitializers: BeforeComma
+BreakConstructorInitializers: AfterColon


The appearance of simple constructors did not look good with former setting:
Before:

Point() : x(0) , y(0) , z(0) { }

After:
Point() : x(0), y(0), z(0) {}

src/lib/include/xtgeo/geometry.hpp

jcrivenaes · 2025-01-23T08:00:26Z

src/lib/include/xtgeo/grid3d.hpp

-                                   const double bot_yinc,
-                                   const double bot_rotation,
-                                   const py::array_t<double> &bot_values);
+get_gridprop_value_between_surfaces(const Grid &grd,


An example when the function signature gets much shorter and easier to maintain

jcrivenaes · 2025-01-23T08:04:44Z

tests/test_internal/test_regsurf.py

+    if num_threads < available_threads / 2:
+        assert np.array_equal(top, keep_top_store["keep_top"]), (
+            f"Mismatch with {num_threads} threads"
+        )


Adjusted this test as it is flaky depending on the actual threads available (consider this is a "hot-fix"; a deeper understanding of cause of flakiness should be considered, but with rather low-priority)

ErichSuter · 2025-01-23T15:15:38Z

src/lib/src/common/geometry/volumes.cpp

+          { corners.lower_se.x, corners.lower_se.y, corners.lower_se.z },
+          { corners.lower_nw.x, corners.lower_nw.y, corners.lower_nw.z },
+          { corners.lower_ne.x, corners.lower_ne.y, corners.lower_ne.z } }
+    };


Don't know if it is important, but for clarity it could be an idea to implement an iterator over the corners in CellCorners (I assume that is possible in C++). It could consist of two iterators for the top and base quadrilaterals (I noticed that somewhere else an anti-clockwise ordering was used).
But that is pretty much what CellCorners.arrange_corners() does😊

ErichSuter · 2025-01-23T16:29:47Z

tests/test_internal/test_geometry.py

-    c2 = [100.0, 50.0, 0.0]
-    c3 = [75.0, 100.0, 0.0]
-    c4 = [100.0, 100.0, 0.55]
+    """Test the interpolate_z_4p_regular function, which returns a float from C++."""


Personally, I like these kinds of drawings directly in the code 😉

ErichSuter

Some really nice object-oriented clean-up here, far easier to read, debug, apply, and to add new functionalities. Good to go from my side 👍

mferrera

Much nicer function signatures. A lot of progress toward the C++ representation of these objects. Nice work

mferrera · 2025-01-27T05:56:10Z

src/lib/include/xtgeo/cube.hpp

-    m_cube.def("cube_stats_along_z", &cube_stats_along_z,
-               "Compute various statistics for cube along the Z axis, returning maps.");
+    py::class_<Cube>(m_cube, "Cube")
+      .def(py::init<>())


Suggested change

.def(py::init<>())

Is there a use case for the default constructor here? (It would allow making a C++ cube like _internal.cube.Cube()). Probably we don't want this since empty constructors were deprecated anyway, and should expect its made from the Python object only. Unless of course it breaks something when removed..

mferrera · 2025-01-27T06:04:33Z

src/lib/include/xtgeo/types.hpp

+struct Points  // placeholder. Consider naming it PointSet for clarity
+{
+    std::vector<Point> points;
+
+    // Default constructor
+    Points() = default;
+
+    // Constructor that takes a vector of points
+    Points(const std::vector<Point> &points) : points(points) {}
+
+    // Method to add a point to the points
+    void add_point(const Point &point) { points.push_back(point); }
+
+    // Method to get the number of points
+    size_t size() const { return points.size(); }
+};  // struct Points


Consider removing and adding when it's being used (i.e. don't add dead code)

mferrera · 2025-01-27T06:04:50Z

src/lib/include/xtgeo/types.hpp

+struct Polygons  // placeholder. Consider naming it PolygonSet for clarity
+{
+    std::vector<Polygon> polygons;
+    std::vector<int> ids;
+
+    // Default constructor
+    Polygons() = default;
+
+    // Method to add a polygon with an ID
+    void add_polygon(const Polygon &polygon, int id)
+    {
+        polygons.push_back(polygon);
+        ids.push_back(id);
+    }
+
+    // Method to get the number of polygons
+    size_t size() const { return polygons.size(); };
+};  // struct Pol


Consider removing and adding when it's being used (i.e. don't add dead code)

mferrera · 2025-01-27T06:10:53Z

src/lib/include/xtgeo/types.hpp

@@ -0,0 +1,337 @@
+// separate header file for structs etc used in xtgeo to avoid circular dependencies


A bit worried this header becomes a kitchen sink

mferrera · 2025-01-27T06:12:49Z

src/lib/include/xtgeo/types.hpp

+    // Default constructor
+    Polygon() = default;


Is a default constructor Polygon() needed here? Could set it to delete if it should always be created with an array

mferrera · 2025-01-27T07:15:25Z

tests/test_grid3d/test_grid.py

-    assert grd.dimensions == bulkvol.dimensions
-    assert np.allclose(cellvol_rms.values, bulkvol.values)
+    # assert grd.dimensions == bulkvol.dimensions
+    # assert np.allclose(cellvol_rms.values, bulkvol.values)


Still meant to be commented out?

mferrera · 2025-01-27T07:15:42Z

tests/test_grid3d/test_grid.py

+    print(bulkvol.values.size)
+    for i, val in enumerate(bulkvol.values.flatten().tolist()):
+        print(i, val)
+


Debugging prints?

mferrera · 2025-01-27T07:16:00Z

tests/test_internal/test_cell_grid3d.py

    assert isinstance(corners, list)
+    print(corners)


Debug print?

mferrera · 2025-01-27T07:17:11Z

tests/test_internal/test_geometry.py

-    volume = _internal.geometry.hexahedron_volume(corners, prec)
-    assert volume == pytest.approx(468.75, rel=1e-3)  # 468.75 is RMS' value


Is the volume check no longer relevant?

mferrera · 2025-01-27T07:20:39Z

src/lib/include/xtgeo/types.hpp

+    // Constructor that takes a Python object and a metadata_only flag
+    RegularSurface(const py::object &rs, bool metadata_only = false)


I'm a bit confused about metadata only, maybe because of thinking so much about dataio... it's meant just to take fields but not the full value-data?

jcrivenaes marked this pull request as draft January 16, 2025 14:35

jcrivenaes force-pushed the testing-shadow-classes-cpp branch 4 times, most recently from 21152af to 4e86605 Compare January 23, 2025 06:37

CLN: move towards structs in C++/pybind11

7d0d54e

jcrivenaes force-pushed the testing-shadow-classes-cpp branch from 4e86605 to 7d0d54e Compare January 23, 2025 07:53

jcrivenaes commented Jan 23, 2025

View reviewed changes

src/lib/include/xtgeo/geometry.hpp Show resolved Hide resolved

jcrivenaes commented Jan 23, 2025

View reviewed changes

jcrivenaes changed the title ~~WIP: refactor C++/pybind11 single variables to structs~~ CLN: refactor C++/pybind11 single variables to structs Jan 23, 2025

jcrivenaes marked this pull request as ready for review January 23, 2025 08:22

jcrivenaes requested a review from mferrera January 23, 2025 08:22

ErichSuter self-requested a review January 23, 2025 08:59

ErichSuter reviewed Jan 23, 2025

View reviewed changes

ErichSuter approved these changes Jan 23, 2025

View reviewed changes

mferrera approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: refactor C++/pybind11 single variables to structs #1294

CLN: refactor C++/pybind11 single variables to structs #1294

jcrivenaes commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 17, 2025 •

edited

Loading

jcrivenaes Jan 23, 2025

jcrivenaes Jan 23, 2025

jcrivenaes Jan 23, 2025

ErichSuter Jan 23, 2025 •

edited

Loading

ErichSuter Jan 23, 2025

ErichSuter left a comment

mferrera left a comment

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

mferrera Jan 27, 2025

		@@ -0,0 +1,337 @@
		// separate header file for structs etc used in xtgeo to avoid circular dependencies

		volume = _internal.geometry.hexahedron_volume(corners, prec)
		assert volume == pytest.approx(468.75, rel=1e-3) # 468.75 is RMS' value

		// Constructor that takes a Python object and a metadata_only flag
		RegularSurface(const py::object &rs, bool metadata_only = false)

CLN: refactor C++/pybind11 single variables to structs #1294

Are you sure you want to change the base?

CLN: refactor C++/pybind11 single variables to structs #1294

Conversation

jcrivenaes commented Jan 16, 2025 • edited Loading

codecov-commenter commented Jan 17, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErichSuter Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErichSuter left a comment

Choose a reason for hiding this comment

mferrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrivenaes commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 17, 2025 •

edited

Loading

ErichSuter Jan 23, 2025 •

edited

Loading