Merge pull request #1597 from emlys/feature/routing-refactor-compare

Reimplement managed raster class in C++

.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
           ls -la dist
           pip install $(find dist -name "natcap.invest*.whl")
 
@@ -180,12 +180,12 @@ jobs:
           # .cpp files in addition to the .pyx files.
           #
           # Elevating any python warnings to errors to catch build issues ASAP.
-          python -W error -m build --sdist
+          NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" python -W error -m build --sdist
 
       - name: Install from source distribution
         run : |
           # Install natcap.invest from the sdist in dist/
-          pip install $(find dist -name "natcap[._-]invest*")
+          NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" pip install $(find dist -name "natcap[._-]invest*")
 
           # Model tests should cover model functionality, we just want
           # to be sure that we can import `natcap.invest` here.
@@ -235,7 +235,7 @@ jobs:
           requirements: ${{ env.CONDA_DEFAULT_DEPENDENCIES }} pytest
 
       - name: Make install
-        run: make install
+        run: NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" make install
 
       - name: Validate sample data
         run: make validate_sampledata
@@ -267,7 +267,7 @@ jobs:
           requirements: ${{ env.CONDA_DEFAULT_DEPENDENCIES }}
 
       - name: Make install
-        run: make install
+        run: NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" make install
 
       - name: Set up node
         uses: actions/setup-node@v3
@@ -328,7 +328,7 @@ jobs:
           requirements: ${{ env.CONDA_DEFAULT_DEPENDENCIES }} pandoc
 
       - name: Make install
-        run: make install
+        run: NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library" make install
 
         # Not caching chocolatey packages because the cache may not be reliable
         # https://github.com/chocolatey/choco/issues/2134

README.rst

@@ -105,6 +105,11 @@ Building InVEST Distributions
 -----------------------------
 
 Once the required tools and packages are available, we can build InVEST.
+On Windows, you must indicate the location of the GDAL libraries with the environment
+variable ``NATCAP_INVEST_GDAL_LIB_PATH``. If you are using conda to manage dependencies
+as we recommend, you can add ``NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library"`` to
+the commands below. (On Mac and Linux, the gdal library path is determined for you
+automatically using ``gdal-config``, which isn't available on Windows.)
 
 
 Building ``natcap.invest`` python package

requirements-dev.txt

@@ -14,7 +14,7 @@
 
 virtualenv>=12.0.1
 pytest
-pytest-subtests
+pytest-subtests<0.14.0
 wheel>=0.27.0
 pypiwin32; sys_platform == 'win32'  # pip-only
 

setup.py

@@ -15,14 +15,25 @@
                  if (not req.startswith(('#', 'hg+', 'git+'))
                      and len(req.strip()) > 0)]
 
-# Since OSX Mavericks, the stdlib has been renamed.  So if we're on OSX, we
-# need to be sure to define which standard c++ library to use.  I don't have
-# access to a pre-Mavericks mac, so hopefully this won't break on someone's
-# older system.  Tested and it works on Mac OSX Catalina.
 compiler_and_linker_args = []
-if platform.system() == 'Darwin':
-    compiler_and_linker_args = ['-stdlib=libc++']
-
+include_dirs = [numpy.get_include(), 'src/natcap/invest/managed_raster']
+if platform.system() == 'Windows':
+    compiler_args = ['/std:c++20']
+    if 'NATCAP_INVEST_GDAL_LIB_PATH' not in os.environ:
+        raise RuntimeError(
+            'env variable NATCAP_INVEST_GDAL_LIB_PATH is not defined. '
+            'This env variable is required when building on Windows. If '
+            'using conda to manage your gdal installation, you may set '
+            'NATCAP_INVEST_GDAL_LIB_PATH="$CONDA_PREFIX/Library".')
+    library_dirs = [f'{os.environ["NATCAP_INVEST_GDAL_LIB_PATH"]}/lib']
+    include_dirs.append(f'{os.environ["NATCAP_INVEST_GDAL_LIB_PATH"]}/include')
+else:
+    library_dirs = []
+    compiler_args = []
+    compiler_and_linker_args = ['-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
 
 class build_py(_build_py):
     """Command to compile translation message catalogs before building."""
@@ -50,13 +61,14 @@ def run(self):
         Extension(
             name=f'natcap.invest.{package}.{module}',
             sources=[f'src/natcap/invest/{package}/{module}.pyx'],
-            include_dirs=[numpy.get_include()] + ['src/natcap/invest/managed_raster'],
-            extra_compile_args=compiler_args + compiler_and_linker_args,
+            include_dirs=include_dirs,
+            extra_compile_args=compiler_args + package_compiler_args + compiler_and_linker_args,
             extra_link_args=compiler_and_linker_args,
             language='c++',
+            libraries=['gdal'],
+            library_dirs=library_dirs,
             define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")]
-        ) for package, module, compiler_args in [
-            ('managed_raster', 'managed_raster', []),
+        ) for package, module, package_compiler_args in [
             ('delineateit', 'delineateit_core', []),
             ('recreation', 'out_of_core_quadtree', []),
             # clang-14 defaults to -ffp-contract=on, which causes the

src/natcap/invest/managed_raster/LRUCache.h

@@ -8,66 +8,72 @@
 using namespace std;
 
 template <class KEY_T, class VAL_T,
-    typename ListIter = typename list< pair<KEY_T,VAL_T> >::iterator,
-    typename MapIter = typename map<KEY_T, ListIter>::iterator > class LRUCache{
-private:
-    // item_list keeps track of the order of which elements have been accessed
-    // element at begin is most recent, element at end is least recent.
-    // first element in the pair is its key while the second is the element
-    list< pair<KEY_T,VAL_T> > item_list;
-    // item_map maps an element's key to its location in the `item_list`
-    // used to make lookups O(log n) time
-    map<KEY_T, ListIter> item_map;
-    size_t cache_size;
-private:
-    void clean(list< pair<KEY_T, VAL_T> > &removed_value_list){
-        while(item_map.size()>cache_size){
-            ListIter last_it = item_list.end(); last_it --;
-            removed_value_list.push_back(
-                make_pair(last_it->first, last_it->second));
-            item_map.erase(last_it->first);
-            item_list.pop_back();
-        }
-    };
-public:
-    LRUCache(int cache_size_):cache_size(cache_size_){
-            ;
-    };
+  typename ListIter = typename list<pair<KEY_T,VAL_T>>::iterator,
+  typename MapIter = typename map<KEY_T, ListIter>::iterator> class LRUCache {
+ private:
+  // item_list keeps track of the order of which elements have been accessed
+  // element at begin is most recent, element at end is least recent.
+  // first element in the pair is its key while the second is the element
+  list<pair<KEY_T,VAL_T>> item_list;
+  // item_map maps an element's key to its location in the `item_list`
+  // used to make lookups O(log n) time
+  map<KEY_T, ListIter> item_map;
+  size_t cache_size;
 
-    ListIter begin() {
-        return item_list.begin();
+  void clean(list<pair<KEY_T, VAL_T>> &removed_value_list) {
+    while(item_map.size() > cache_size) {
+      ListIter last_it = item_list.end();
+      last_it--;
+      removed_value_list.push_back(
+        make_pair(last_it->first, last_it->second));
+      item_map.erase(last_it->first);
+      item_list.pop_back();
     }
+  };
+ public:
+  LRUCache(int cache_size_):cache_size(cache_size_) {
+      ;
+  };
 
-    ListIter end() {
-        return item_list.end();
+  ListIter begin() {
+    return item_list.begin();
+  }
+
+  ListIter end() {
+    return item_list.end();
+  }
+
+  // Insert a new key-value pair into the cache.
+  void put(
+      const KEY_T &key, const VAL_T &val,
+      list<pair<KEY_T, VAL_T>> &removed_value_list) {
+    MapIter it = item_map.find(key);
+    if(it != item_map.end()){
+      // it's already in the cache, delete the location in the item
+      // list and in the lookup map
+      item_list.erase(it->second);
+      item_map.erase(it);
     }
+    // insert a new item in the front since it's most recently used
+    item_list.push_front(make_pair(key, val));
+    // record its iterator in the map
+    item_map.insert(make_pair(key, item_list.begin()));
+    // possibly remove any elements that have exceeded the cache size
+    return clean(removed_value_list);
+  };
+
+  // Return whether a key exists in the cache.
+  bool exist(const KEY_T &key) {
+    return (item_map.count(key) > 0);
+  };
 
-    void put(
-            const KEY_T &key, const VAL_T &val,
-            list< pair<KEY_T, VAL_T> > &removed_value_list) {
-        MapIter it = item_map.find(key);
-        if(it != item_map.end()){
-            // it's already in the cache, delete the location in the item
-            // list and in the lookup map
-            item_list.erase(it->second);
-            item_map.erase(it);
-        }
-        // insert a new item in the front since it's most recently used
-        item_list.push_front(make_pair(key,val));
-        // record its iterator in the map
-        item_map.insert(make_pair(key, item_list.begin()));
-        // possibly remove any elements that have exceeded the cache size
-        return clean(removed_value_list);
-    };
-    bool exist(const KEY_T &key){
-        return (item_map.count(key)>0);
-    };
-    VAL_T& get(const KEY_T &key){
-        MapIter it = item_map.find(key);
-        assert(it!=item_map.end());
-        // move the element to the front of the list
-        item_list.splice(item_list.begin(), item_list, it->second);
-        return it->second->second;
-    };
+  // Return the cached value associated with a key.
+  VAL_T& get(const KEY_T &key) {
+    MapIter it = item_map.find(key);
+    assert(it != item_map.end());
+    // move the element to the front of the list
+    item_list.splice(item_list.begin(), item_list, it->second);
+    return it->second->second;
+  };
 };
 #endif