Skip to content

Fix error in ReprojectRasterExtent - #2825

Merged
pomadchin merged 4 commits into
locationtech:masterfrom
jpolchlo:fix/reproject-raster-extent
Nov 28, 2018
Merged

Fix error in ReprojectRasterExtent#2825
pomadchin merged 4 commits into
locationtech:masterfrom
jpolchlo:fix/reproject-raster-extent

Conversation

@jpolchlo

@jpolchlo jpolchlo commented Nov 17, 2018

Copy link
Copy Markdown
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

jpolchlo commented Nov 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@jpolchlo jpolchlo mentioned this pull request Nov 19, 2018
Closed
pomadchin
pomadchin reviewed Nov 26, 2018
View reviewed changes

@pomadchin pomadchin left a comment

Copy link
Copy Markdown
Member

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 force-pushed the fix/reproject-raster-extent branch from d79255b to 9399d87 Compare November 27, 2018 18:56
@pomadchin pomadchin mentioned this pull request Nov 27, 2018
Merged
@pomadchin
pomadchin merged commit d2761bb into locationtech:master Nov 28, 2018
echeipesh pushed a commit that referenced this pull request Dec 28, 2018
@pomadchin @echeipesh
Merge pull request #2825 from jpolchlo/fix/reproject-raster-extent
e591518 
Fix error in `ReprojectRasterExtent`
FireByTrial added a commit to FireByTrial/geotrellis that referenced this pull request Nov 25, 2020
@FireByTrial
Revert "Merge pull request locationtech#2825 from jpolchlo/fix/reproj…
a31f411 
…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

Reviewers

@pomadchin pomadchin pomadchin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jpolchlo @pomadchin