Reimplement managed raster class in C++ #1597

emlys · 2024-07-17T17:59:19Z

Description

This PR reimplements the ManagedRaster class in C++ for better performance. Tested with sdr.calculate_sediment_deposition and the SDR sample data, it's now just slightly faster than the original implementation on main (0.59 vs 0.61 seconds), whereas the python implementation on feature/routing-refactor takes nearly twice as long.

I'm a beginner at C++ so I'm sure I'm not always using the right conventions or design patterns, but this seems like an okay starting point. I took some guidance from the Google C++ style guide. Ignoring some guidelines that don't seem to make sense in this context. For instance, I have all the code in a header file because splitting it out into ManagedRaster.h and ManagedRaster.cpp significantly slowed it down.

Happy to discuss and walk through this PR on a zoom call as well.

Checklist

Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
Updated the user's guide (if needed)
Tested the Workbench UI (if relevant)

…-compare

emlys · 2024-11-05T19:46:05Z

@phargogh this is finally ready for re-review! The biggest change is I figured out how to implement the iterators properly, so they can be iterated with the usual syntax, solving the issues with tracking/incrementing values properly.

phargogh

Thanks @emlys , this looks good to me! I had a couple of minor comments, and then I also wanted to confirm: what we have here in this PR looks to be intended for MFD routing, specifically, so is D8 routing to be added in a future PR?

phargogh · 2024-11-18T18:37:01Z

setup.py

+include_dirs = [numpy.get_include(), 'src/natcap/invest/managed_raster']
+if platform.system() == 'Windows':
+    compiler_args = ['/std:c++20']
+    library_dirs = [f'{os.environ["CONDA_PREFIX"]}/Library/lib']
+    include_dirs.append(f'{os.environ["CONDA_PREFIX"]}/Library/include')
+else:
+    library_dirs = []
+    compiler_args = []
+    compiler_and_linker_args = ['-stdlib=libc++', '-std=c++20']
+    library_dirs = [subprocess.run(
+        ['gdal-config', '--libs'], capture_output=True, text=True
+    ).stdout.split()[0][2:]]  # get the first argument which is the library path


Since we are now building directly against gdal, should we add the new build dependency to pyproject.toml?

Hmmm... the libraries are a dependency of the build, but the gdal python bindings themselves aren't necessary. I'm not sure there's any standardized way to document that kind of dependency?

phargogh · 2024-11-18T18:58:36Z

setup.py

+else:
+    library_dirs = []
+    compiler_args = []
+    compiler_and_linker_args = ['-stdlib=libc++', '-std=c++20']


Is C++20 truly the minimum language version required? I don't think this should be a huge deal since compilers seem to pretty rapidly adopt the new standards, so I think this is mostly a question of curiosity.

Good question, ~~I think I used some C++20 features during development, but now only need C++17. (It still compiles with 11, but I get warnings that the inline variables are a C++17 feature.) Updated in d50a2c9~~ C++20 is needed on the Windows build only, for some reason, otherwise there are a lot of syntax errors, e.g. https://github.com/natcap/invest/actions/runs/12245867222/job/34160661279

Sounds good! Thanks for looking into it.

If I had to guess, maybe the lack of an --ffp-contract=off flag (needed for addressing a compilation bug in viewshed) is causing the rest of the errors? If (big if) that's the only thing standing in the way, then the viewshed tests would fail, which isn't worth it if all we need is a recent enough version of the C++ standard.

phargogh · 2024-11-18T19:00:23Z

src/natcap/invest/managed_raster/ManagedRaster.h

+    ManagedFlowDirRaster(char* raster_path, int band_id, bool write_mode)
+        : ManagedRaster(raster_path, band_id, write_mode)   // Call the superclass constructor in the subclass' initialization list.
+        {
+            // do something with bar
+        }


Is the plan to still do something in this constructor?

Fixed in 25c954a

…-compare

emlys · 2024-12-10T21:12:32Z

@phargogh yes, the D8 feature is over in #1633. I also got the RTD build working here, and added an env variable to configure with GDAL lib path for windows builds.

phargogh

Thanks so much @emlys it's looking good!

phargogh · 2024-12-12T02:07:50Z

.github/workflows/build-and-test.yml

@@ -119,7 +119,7 @@ jobs:
        run: |
          # uninstall InVEST if it was already in the restored cache
          python -m pip uninstall -y natcap.invest
-          python -m build --wheel
+          NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" python -m build --wheel


I see we're repeating this new environment variable in a couple places, so would it interfere with anything to move it to the env block so we only need to define it once? I'm pretty sure we can to a top-level env: mapping to allow the env var to propagate across all of the different sections.

Nope, I see now that we won't know what $CONDA_PREFIX is going to be until after we're in the activated environment, so this seems like the most direct way to define it. 👍

phargogh · 2024-12-12T02:12:13Z

setup.py

+else:
+    library_dirs = []
+    compiler_args = []
+    compiler_and_linker_args = ['-stdlib=libc++', '-std=c++20']


Sounds good! Thanks for looking into it.

If I had to guess, maybe the lack of an --ffp-contract=off flag (needed for addressing a compilation bug in viewshed) is causing the rest of the errors? If (big if) that's the only thing standing in the way, then the viewshed tests would fail, which isn't worth it if all we need is a recent enough version of the C++ standard.

emlys added 5 commits May 7, 2024 10:25

compare different implementations of upslope neighbors

5e58f60

improve efficiency of managedraster helper functions

7c51c08

work in progress: rewrite managed raster module in c++

a55d57c

work in progress: managed raster in C++

068d886

C++ managed raster implementation now faster on SDR sample data

8576088

emlys changed the base branch from main to feature/routing-refactor July 17, 2024 18:00

emlys added 24 commits July 17, 2024 11:18

Merge branch 'feature/routing-refactor' into feature/routing-refactor…

7f2ec80

…-compare

NDR tests passing with new C++ implementation of managedraster

11ef40f

all tests using managedraster passing locally

c7486f2

debugging: find gdal.h

048e769

debugging: add incldue path to compile args for windows

cccf0e6

debugging

8d104a4

debugging'

6ca0f78

debugging

61d1f65

debugging

d3aebac

debugging

eb15fa1

debugging

93ec4b9

debugging

4506414

debugging

1ff72b6

debugging

6acaea6

use C++20; no need to compile managed_raster as a separate extension

9316d20

debugging

0d4805a

debugging

53b15d0

debugging

06c8648

debugging

1749eb9

debugging

e4c234f

debugging

bbd7c5a

debugging

7bc4b58

debugging

efc5527

debugging

474a89a

emlys added 12 commits October 30, 2024 19:21

debugging

7b9db1f

debugging

9071362

debugging

ba9cebe

debugging

13cc0dc

use const reference parameters to equality fns

7a767f5

inherit from Neighbors class

5b8c04e

inherit operator funcs

947900c

inherit equality functions from parent class

f737225

run all tests

3a623eb

inherit increment operators from parent class

420145f

call base class constructors where possible

dde78df

setup.py: use same options for linux as for mac

11cc0ef

emlys requested a review from phargogh November 5, 2024 19:46

phargogh reviewed Nov 18, 2024

View reviewed changes

emlys added 7 commits November 22, 2024 11:54

remove extra comments

25c954a

downgrade compiler version to c++17

d50a2c9

read gdal lib path from env variable on windows

3adae2d

set NATCAP_INVEST_GDAL_LIB_PATH in github actions

24d1028

bump c++ version back to 20

b3d2a55

set NATCAP_INVEST_GDAL_LIB_PATH in one more place

45ec188

temporarily pin pytest-subtests version

9b95796

emlys force-pushed the feature/routing-refactor-compare branch from c01339a to 9b95796 Compare December 10, 2024 03:21

emlys added 3 commits December 9, 2024 19:21

Merge branch 'feature/routing-refactor' into feature/routing-refactor…

3c9b795

…-compare

remove stdlib flag from compile command; default to libstdc++

07059b0

remove managed_raster pyx file

041cd1a

emlys requested a review from phargogh December 10, 2024 21:12

phargogh approved these changes Dec 12, 2024

View reviewed changes

phargogh merged commit df352de into natcap:feature/routing-refactor Dec 12, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement managed raster class in C++ #1597

Reimplement managed raster class in C++ #1597

emlys commented Jul 17, 2024 •

edited

Loading

emlys commented Nov 5, 2024

phargogh left a comment

phargogh Nov 18, 2024

emlys Dec 9, 2024

phargogh Nov 18, 2024

emlys Dec 9, 2024 •

edited

Loading

phargogh Dec 12, 2024

phargogh Nov 18, 2024

emlys Nov 22, 2024

emlys commented Dec 10, 2024

phargogh left a comment

phargogh Dec 12, 2024

phargogh Dec 12, 2024

phargogh Dec 12, 2024

Reimplement managed raster class in C++ #1597

Reimplement managed raster class in C++ #1597

Conversation

emlys commented Jul 17, 2024 • edited Loading

Description

Checklist

emlys commented Nov 5, 2024

phargogh 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

emlys Dec 9, 2024 • edited Loading

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

emlys commented Dec 10, 2024

phargogh 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

emlys commented Jul 17, 2024 •

edited

Loading

emlys Dec 9, 2024 •

edited

Loading