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

Fix error in ReprojectRasterExtent #2825

Merged
merged 4 commits into from
Nov 28, 2018

Conversation

jpolchlo
Copy link
Contributor

Overview

While investigating a reprojection issue in geotrellis-contrib, it came to my attention that the raster extents produced by ReprojectRasterExtent are off. Consider the following: using https://github.com/geotrellis/geotrellis-contrib/blob/master/vlm/src/test/resources/img/aspect-tiled.tif as the base of this example, the following definitions apply:

val tiff = GeoTiffReader.readMultiband(getByteReader("aspect-tiled.tif"), streaming = true)
val targetRE = ReprojectRasterExtent(tiff.rasterExtent, tiff.crs, WebMercator)
val region = tiff.projectedExtent.reprojectAsPolygon(WebMercator, 0.001)

We then note that targetRE.extent and region.envelope diverge:

targetRE.extent: geotrellis.vector.Extent = Extent(-8769150.640916323, 4257706.9716441, -8750638.524926396, 4274455.441673627)
region.envelope: geotrellis.vector.Extent = Extent(-8769150.640916323, 4257707.61418559, -8750636.281752571, 4274455.441673627)

Note that ymin and xmax differ, and that the tolerance value of 0.001 has no effect, since the corners of the region are on the extent boundary.

I believe that the region is the authoritative source of truth. So, the reprojected raster extent should tightly bound this polygon.

Signed-off-by: jpolchlo [email protected]

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • Unit tests added for bug-fix or new feature

@jpolchlo
Copy link
Contributor Author

jpolchlo commented Nov 17, 2018

As of now, some of the original tests for ReprojectRasterExtent are failing due to small variations introduced by this PR. I do believe that the old implementation was incorrect and needed to be fixed, but this is leading to a slight divergence between GDAL and GT, which is what the failing tests check against. We'll need some discussion as to whether the new results are precisely what we intend them to be and whether targeting strict agreement with GDAL is the right thing to do.

Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

Holding this until geotrellis/geotrellis-contrib#46 question would be resolved.

@pomadchin pomadchin merged commit d2761bb into locationtech:master Nov 28, 2018
echeipesh pushed a commit that referenced this pull request Dec 28, 2018
FireByTrial added a commit to FireByTrial/geotrellis that referenced this pull request Nov 25, 2020
…ect-raster-extent"

This reverts commit d2761bb, reversing
changes made to edfc69e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants