Move GDAL constraint into constraints file and update to GDAL 3.8 #1546
Conversation
emlys
changed the title
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I just had one comment about helping future us remember what belongs in the constraints file since we are regularly using it for conda/mamba applications. I'd be happy with a comment in the constraints file, but it could also be interesting to update the conda/mamba conversion script to handle environment markers if you thought that might be a better solution. Totally up to you :)
Aside from that, it looks great to me, thanks @emlys!
constraints_tests.txt
Outdated
| @@ -0,0 +1,5 @@ | |||
| # This file contains package constraints needed to run the invest test suite | |||
There was a problem hiding this comment.
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!
|
Thanks @phargogh, I added a note about the constraints format! |
The version cap was added to avoid problems with our test suite. For details, see how we resolved the issue in natcap/invest#1546
Description
Fixes #916
Create a new file
constraints_tests.txtthat stores dependency constraints needed to run our tests, but not necessarily to use invest in general.Exclude gdal versions 3.6.* and 3.7.* which had the bug.
Fortunately, conda seems to have no problem with multiple requirements for the same package in one environment yaml file. This makes it easy to include the constraints file in our workflow - we can pass it in to the
convert-requirements-to-conda-yml.pyscript just like another requirements file. The resulting yml will contain this:Also includes updates needed for the latest versions of GDAL:
GDT_Int8type and deprecation of thePIXELTYPE=SIGNEDBYTEflag (hooray!)Checklist