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

RasterSource.resolutions becomes List[CellSize] #3124

Merged
merged 4 commits into from
Oct 15, 2019

Conversation

echeipesh
Copy link
Contributor

@echeipesh echeipesh commented Oct 14, 2019

Overview

This PR is intended to address usability issues around resampling RasterSource described in #3100

RasterSource.resolutions is not List[CellSize]. As discussed in the issue this removes the ability to provide GridBounds from RasterSource.resolutions to the RasterSource.read method of the same instance. The judgement call is that if each overview somehow has differing extent this is considered very unusual and can be diagnosed by further digging into RasterMetadata interface or through additional specialized API.

Grid.gridBounds are removed. This added a very confusing case where GridBounds could either be a window in some raster or describe the whole footprint of raster. Removing Grid.gridBounds goes a long way to remove that confusion. This is a recently added member to the class, so impact is judged to be small.

GridBounds.intersects(dimensions) - For those cases when we're checking for intersection with a raster we're using the new Dimensions class. Dimensions can be checked for intersection vs GridBounds and intersection of GridBounds and Dimensions is smaller Option[GridBounds].

Grid.dimensions becomes Dimensions[N] Instead of (N, N) - with addition of specialized API described above. Giving this a class goes a long way to clarify its usage in API. Note that this change results in fix-ups to tests that could be be caught by compiler due to checking for equality with "==". This is likely to happen in client code but should be easily fixed.

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
  • Unit tests added for bug-fix or new feature

Notes

gdal > console
[info] Starting scala interpreter...
Welcome to Scala 2.11.12 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_211).
Type in expressions for evaluation. Or try :help.

scala> import geotrellis.raster.gdal._
import geotrellis.raster.gdal._

scala> import geotrellis.raster.io.geotiff.{AutoHigherResolution, OverviewStrategy}
import geotrellis.raster.io.geotiff.{AutoHigherResolution, OverviewStrategy}

scala> import geotrellis.raster.resample._
import geotrellis.raster.resample._

scala> val rs = GDALRasterSource("/Users/eugene/proj/geotrellis/raster/data/vlm/aspect-tiled.tif")
rs: geotrellis.raster.gdal.GDALRasterSource = GDALRasterSource(/Users/eugene/proj/geotrellis/raster/data/vlm/aspect-tiled.tif,GDALWarpOptions(-of VRT -ovr AUTO))

scala> rs.resolutions
res0: List[geotrellis.raster.CellSize] = List(CellSize(20.0,20.0), CellSize(40.0,39.94082840236686), CellSize(79.7872340425532,79.88165680473372))

Closes #3100

Add Dimensions and use them for intersection check
Other changes not documented because they were changes from 3.0-SNAPSHOT rather than 2.3
@echeipesh
Copy link
Contributor Author

While preparing this PR following issue was discovered: #3123

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.

💯 beatiful!

@@ -317,7 +317,7 @@ class GDALRasterSourceRDDSpec extends FunSpec with TestEnvironment with BeforeAn
val actual = RasterSourceRDD.read(readingSources, floatingLayout).stitch()

// random chip to test agains, to speed up tests
val gridBounds = RasterExtent(randomExtentWithin(actual.extent), actual.cellSize).gridBounds
val gridBounds = actual.rasterExtent.gridBoundsFor(randomExtentWithin(actual.extent))
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised how much better it looks!

@@ -26,7 +26,7 @@ case class GDALMetadata(
bandCount: Int,
cellType: CellType,
gridExtent: GridExtent[Long],
resolutions: List[GridExtent[Long]],
resolutions: List[CellSize],
Copy link
Member

Choose a reason for hiding this comment

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

👍 finally these are resolutions indeed.

}

object Dimensions {
implicit def apply[N: Integral](tup: (N, N)) = new Dimensions(tup._1, tup._2)
Copy link
Member

Choose a reason for hiding this comment

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

Just a notion: boxing happens here, but we don't care too much about it.

@pomadchin pomadchin merged commit aa4806e into master Oct 15, 2019
@echeipesh echeipesh deleted the refactor/rastersource-resolutions-2 branch October 16, 2019 22:14
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.

RasterSource API feedback on resolutions member
2 participants