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

Move GDAL constraint into constraints file and update to GDAL 3.8 #1546

Merged
merged 13 commits into from
Apr 3, 2024
Merged
3 changes: 1 addition & 2 deletions .github/workflows/build-and-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ jobs:
with:
fetch-depth: 0 # Fetch complete history for accurate versioning


# NOTE: It takes twice as long to save the sample data cache
# as it does to do a fresh clone (almost 5 minutes vs. 2.5 minutes)
# Test data is way, way faster by contrast (on the order of a few
Expand All @@ -94,7 +93,7 @@ jobs:
uses: ./.github/actions/setup_env
with:
python-version: ${{ matrix.python-version }}
requirements-files: requirements.txt requirements-dev.txt
requirements-files: requirements.txt requirements-dev.txt constraints_tests.txt
requirements: ${{ env.CONDA_DEFAULT_DEPENDENCIES }}

- name: Download previous conda environment.yml
Expand Down
2 changes: 1 addition & 1 deletion .readthedocs_environment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ channels:
- nodefaults
dependencies:
- python=3.11
- gdal>=3.4.2,<3.6.0
- gdal>=3.4.2
- pip
- pip:
- -r requirements.txt
Expand Down
5 changes: 5 additions & 0 deletions constraints_tests.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This file contains package constraints needed to run the invest test suite
Copy link
Member

Choose a reason for hiding this comment

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

Since the concept of a constraints file is pip-specific (I think?) but we're using conda/mamba to manage our environments, would it be worth adding a note here about the syntax that is allowed in this file?

I'm guessing environment markers might be a big one that wouldn't be supported without further work on our requirements conversion script, for example!


# A gdal bug caused our test suite to fail, but this issue is unlikely to
# occur with regular use of invest. https://github.com/OSGeo/gdal/issues/8497
GDAL!=3.6.*,!=3.7.*
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,4 @@ where = ["src"]

[tool.pytest.ini_options]
# raise warnings to errors, except for deprecation warnings
filterwarnings = ["error", "default::DeprecationWarning"]
filterwarnings = ["error", "default::DeprecationWarning", "default::FutureWarning"]
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# scripts/convert-requirements-to-conda-yml.py as though it can only be found
# on pip.

GDAL>=3.4.2,<3.6.0
GDAL>=3.4.2
Pyro4==4.77 # pip-only
pandas>=1.2.1
numpy>=1.11.0,!=1.16.0
Expand Down
9 changes: 4 additions & 5 deletions src/natcap/invest/hra.py
Original file line number Diff line number Diff line change
Expand Up @@ -633,10 +633,6 @@ def execute(args):
# If the input is a vector, reproject to the AOI SRS and simplify.
# Rasterization happens in the alignment step.
elif gis_type == pygeoprocessing.VECTOR_TYPE:
# Habitats and stressors are rasterized with ALL_TOUCHED=TRUE
if name in habitats_info or name in stressors_info:
habitat_stressor_vectors.add(source_filepath)

# Using Shapefile here because its driver appears to not raise a
# warning if a MultiPolygon geometry is inserted into a Polygon
# layer, which was happening on a real-world sample dataset while
Expand Down Expand Up @@ -680,6 +676,10 @@ def execute(args):
dependent_task_list=[reprojected_vector_task]
))

# Habitats and stressors are rasterized with ALL_TOUCHED=TRUE
if name in habitats_info or name in stressors_info:
habitat_stressor_vectors.add(target_simplified_vector)
Copy link
Member Author

Choose a reason for hiding this comment

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

It appears that the intent was to apply ALL_TOUCHED=TRUE when rasterizing habitats and stressors, but that was not happening because target_simplified_vector, not source_filepath, is used.


# Later operations make use of the habitats rasters or the stressors
# rasters, so it's useful to collect those here now.
if name in habitats_info:
Expand Down Expand Up @@ -1648,7 +1648,6 @@ def _simplify(source_vector_path, tolerance, target_vector_path,
for source_feature in source_layer:
target_feature = ogr.Feature(target_layer_defn)
source_geom = source_feature.GetGeometryRef()

simplified_geom = source_geom.SimplifyPreserveTopology(tolerance)
if simplified_geom is not None:
target_geom = simplified_geom
Expand Down
4 changes: 2 additions & 2 deletions src/natcap/invest/ndr/ndr_core.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -415,9 +415,9 @@ def ndr_eff_calculation(

# create direction raster in bytes
def _mfd_to_flow_dir_op(mfd_array):
result = numpy.zeros(mfd_array.shape, dtype=numpy.int8)
result = numpy.zeros(mfd_array.shape, dtype=numpy.uint8)
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this always should've been uint8. I think result is supposed to be an 8-digit binary number indicating which of the 8 directions around a pixel have flow. (i.e. 10000001 means there is flow to neighbors 0 and 7). The max value 11111111 = 255, so uint8 is needed.

I'm not sure exactly why it worked before, but it probably broke because of changes in GDAL's handling of signed 8-bit integers in 3.7.0.

for i in range(8):
result[:] |= (((mfd_array >> (i*4)) & 0xF) > 0) << i
result[:] |= ((((mfd_array >> (i*4)) & 0xF) > 0) << i).astype(numpy.uint8)
return result

pygeoprocessing.raster_calculator(
Expand Down
1 change: 1 addition & 0 deletions tests/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2002,6 +2002,7 @@ def test_spatial_overlap_error(self):
layer = vector.CreateLayer('layer', vector_srs, ogr.wkbPoint)
new_feature = ogr.Feature(layer.GetLayerDefn())
new_feature.SetGeometry(ogr.CreateGeometryFromWkt('POINT (1 1)'))
layer.CreateFeature(new_feature)
Copy link
Member Author

Choose a reason for hiding this comment

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

The feature wasn't being added to the layer, so the layer was actually empty. I'm not sure what determines the extent of an empty layer, but I'm guessing it's undefined behavior. Some change in GDAL (or GEOS or PROJ?) caused the extent to include a nan.

By adding a feature to the layer, we get the expected behavior.


new_feature = None
layer = None
Expand Down
Loading