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

Tests fail with GDAL 3.0.2 #16

Closed
phargogh opened this issue Dec 20, 2019 · 9 comments
Closed

Tests fail with GDAL 3.0.2 #16

phargogh opened this issue Dec 20, 2019 · 9 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@phargogh
Copy link
Member

In a new virtualenv, I pip installed pygeoprocessing==1.9.1 using dependencies (including GDAL 3.0.2 from Gohlke) from our PyPI. 3 of our tests fail. I've attached the log from this test run here: gdal3_error_log.txt

Tests work great on the latest version of GDAL v2 (currently 2.4.2, I believe)

@phargogh phargogh added the bug Something isn't working label Dec 20, 2019
@phargogh
Copy link
Member Author

It's also worth noting that Gohlke isn't producing Python 3.8 wheels for GDAL 2: https://www.lfd.uci.edu/~gohlke/pythonlibs/#gdal, so GDAL3 should really be in our future.

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 4, 2020

Found some interesting things going on with difference between GDAL 3 and GDAL 2.
When transforming bounding box from align_and_resize_raster_stack:
GDAL 2 [Python 3.7]:
target_bbox : [845079.5702749295, -99734.04483739965, 956619.8421206995, 11081.556155365919]
GDAL 3 [Python 3.8]:
target_bbox : [733729.7578060223, 11060.483077897718, 845058.6446509969, 121762.53706604337]

Wanted to document this before signing off for the day.

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 5, 2020

There was some pretty big changes to GDAL 3.0 relating to PROJ 6. Here are some notes: proj6-wkt2. This will take awhile to digest.

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 5, 2020

It looks like the change with the axis order is the issue. Switching the points in pygeo _transform_point from x, y to y, x gives the tested expected results. Supposedly setting wgs84_sr.SetAxisMappingStrategy(osr.OAMS_TRADITIONAL_GIS_ORDER) is supposed to keep the previous order, but I have not gotten it to work yet.

This post sums it up quite well: OSGeo/gdal#1546

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 19, 2020

Came across a fresh error in tests for GDAL 3+

__________________________________________________________________________________ UFRMTests.test_ufrm_regression ___________________________________________________________________________________

self = <test_ufrm.UFRMTests testMethod=test_ufrm_regression>

    def test_ufrm_regression(self):
        """UFRM: regression test."""
        from natcap.invest import urban_flood_risk_mitigation
        args = self._make_args()
        urban_flood_risk_mitigation.execute(args)

        result_vector = gdal.OpenEx(os.path.join(
            args['workspace_dir'], 'flood_risk_service_Test1.shp'),
            gdal.OF_VECTOR)
        result_layer = result_vector.GetLayer()
>       result_feature = next(result_layer)
E       TypeError: 'Layer' object is not an iterator

tests\test_ufrm.py:63: TypeError

@dcdenu4
Copy link
Member

dcdenu4 commented Feb 19, 2020

Instead of using next it might be better to call: GetNextFeature() from : https://gdal.org/python/osgeo.ogr.Layer-class.html#GetNextFeature. This allows the test to pass in GDAL 3.

@phargogh phargogh added this to the 1.10 milestone Feb 19, 2020
@dcdenu4
Copy link
Member

dcdenu4 commented Feb 20, 2020

GDAL 3 expected test outputs will vary slightly in some cases from GDAL 2. Should we support both GDAL 2 and GDAL 3 test cases?

dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 24, 2020
Adding support to handle GDAL 3. This effectively ends support for
GDAL 2. When using coordinate transformations now setting Axis
properties to be 'friendly' which means to expect lon,lat as opposed to
the now standard of lat,lon. This was recommonended in the migration
guide from GDAL.

Changing the values of a hardcoded test slightly as spatial references
are slightly different in GDAL3, so minor extent differences are
expected in some instances.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 24, 2020
Adding python 3.8 to setup.py for support as all GDAL 3 testing has been
done using python 3.8
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 24, 2020
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 24, 2020
@dcdenu4
Copy link
Member

dcdenu4 commented Feb 25, 2020

Below is a post I made to the premature PR for this issue:

I just spent a significant amount of time in the WKT2 docs, PROJ 6 docs, and GDAL 3 Docs. I think I'm starting to develop a deeper understanding. I think the basics are as follows:

  • GDAL 3 conforms to the authority defining coordinate reference systems (CRS).
  • For Geographic CRS (WGS84), this means no longer ignoring Axis Order but abiding by LAT,LON order
  • For Projected CRS (UTM), most EPSG in the registry use Easting, Northing order
  • We think of things in general as x,y because it is convenient, but now a Transform expects Axis Order, where if long=x and lat=y, GDAL 3 would take Transform(y, x)
  • Thus when transforming from Projected CRS to Geographic CRS, Transform(x,y) will return y,x (Lat,Lon)
  • Thus when transforming from Geographic CRS to Projected CRS, Transform(y,x) will return x,y (Easting, Northing)
  • Thus when transforming from Geographic CRS to Geographic CRS, Transform(y,x) will return y,x (Lat,Lon)
  • Thus when transforming from Projected CRS to Projected CRS, Transform(x,y) will return x,y (Easting, Northing)

note: Most projected CRS use Easting first, Northing second convention, however, some defined in the EPSG registry use the reverse convention.

So, I think you're correct in that not.____.IsProjected() is probably wrong, but _____.IsGeographic() could be OK. Basically that Axis Order setting SetAxisMappingStrategy(osr.OAMS_TRADITIONAL_GIS_ORDER) is saying, if we've got a Geographic CRS, use Lon,Lat like the good old days (since we think of lon,lat as x,y. We certainly can, but maybe should, NOT do that.

Perhaps a better solution is to create a mapping of Axis Order to Geographic (Lat,Lon) and Projected (Easting, Northing), such that when handling the extents and bounding box points we can correctly return x, y.

dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 27, 2020
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 27, 2020
There is no need to check if the SRS is geographic or projected,
GDAL 3 appears to only swap when expected, like for lat/lon
to lon/lat for GIS friendly axis order.

Also removing the axis swapping in reproject_vector where the
geometries actually handle this internally.
Adding better comments.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 27, 2020
This is needed because when the geometries are transformed to
the target SRS, by default GDAL 3 will now return those as
Lat,Lon, however they're handled as Lon,Lat. Setting this
has the transformed geometries returned as Lon,Lat if using a
geographic SRS
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 27, 2020
Adding tests to transform_bounding_box and reproject_vector functions
where osr.CreateCoordinateTransformation are handled. The tests for
transform bounding box use coordinates from a polygon made in QGIS
in the New England Coastal area for WGS84 and UTM 19N. The expected
results were varified in QGIS.

The test for reproject vector handle all possible transformation cases
in and out of WGS84 and Projected SRS. Specifically the latlon to latlon
test checks that the vectors are identical as well as the SRS is as
expected. To handle WGS84 I added a WGS84 reference to sampledata,
following the conventions for the other used SRS.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 28, 2020
Refactoring reproject_vector test functions to create their own vectors
for transforming.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Feb 28, 2020
Like raster creation options, exposing OSR Axis Order Mapping Strategy
for creating coordinate transformations in def transform_bounding_box
and def reproject_vector.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Mar 3, 2020
Also adding conda-forge and anaconda to conda channels
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Mar 3, 2020
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Mar 10, 2020
Using setup-miniconda as opposed to setup-conda as this resource
properly sets the GDAL_DATA and PROJ_LIB envs. Also setting GDAL to
3.0.4 as has better precision handling than 3.0.2 which is likely
related to bug fixes in handling SRS and forcing transformations in and
out of WGS84.
dcdenu4 added a commit to dcdenu4/pygeoprocessing that referenced this issue Mar 12, 2020
@dcdenu4
Copy link
Member

dcdenu4 commented Mar 18, 2020

I'm closing this issue since the GDAL 3 PR has been merged and since there is another issue #44 that reflects why the tests are failing.

@dcdenu4 dcdenu4 closed this as completed Mar 18, 2020
@dcdenu4 dcdenu4 reopened this Mar 18, 2020
@dcdenu4 dcdenu4 closed this as completed Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants