Generate Windows That Conform To GeoTiff Segments by jamesmcclain · Pull Request #2402 · locationtech/geotrellis

jamesmcclain · 2017-10-02T23:52:42Z

Ensure that S3 window size is being properly interpreted (is windowsSize a length or an area in this context?)
Ensure that handling of striped segments is viable (are tiles of one pixel in height okay?)

Connects #2173

echeipesh · 2017-10-06T16:20:39Z

-        val segments: RDD[(String, Array[GridBounds])] =
-          sourceGeoTiffInfo.segmentsByPartitionBytes(partitionBytes, windowSize)
+      case (_, Some(partitionBytes)) => {
+        val maxSize = math.min(options.maxTileSize.getOrElse(1<<10), windowSize.getOrElse(1<<10)) // XXX is windowSize a length or an area?


windowSize is length, implicitly for square windows.

Why can't we write here just 1024 instead of 1 << 10?

Don't think that getOrElse is a good idea here, as it can hide a possible user error. What logic do you want to follow here? Mb we can add these attributes into the match function above? match supports logical or too.

windowSize is length

echeipesh · 2017-10-06T16:25:07Z

Ensure that handling of striped segments is viable (are tiles of one pixel in height okay?)

It seems strange but the thinking went is that if you don't have an opinion about the shape of tiles you want you're probably using this RDD as input to tiler. From the perspective of optimizing for IO reading the segments as skinny tiles is fine, from the perspective of tiler chunking a skinny tile into multiple tiles is not problematic either.

jamesmcclain · 2017-10-06T19:25:59Z

It seems strange but the thinking went is that if you don't have an opinion about the shape of tiles you want you're probably using this RDD as input to tiler. From the perspective of optimizing for IO reading the segments as skinny tiles is fine, from the perspective of tiler chunking a skinny tile into multiple tiles is not problematic either.

Sounds capital.

pomadchin

It works fine! A couple of comments and most of them are related to code style / api questions.

pomadchin · 2017-10-10T12:43:38Z

-        val segments: RDD[(String, Array[GridBounds])] =
-          sourceGeoTiffInfo.segmentsByPartitionBytes(partitionBytes, windowSize)
+      case (_, Some(partitionBytes)) => {
+        val maxSize = math.min(options.maxTileSize.getOrElse(1<<10), windowSize.getOrElse(1<<10)) // XXX is windowSize a length or an area?


Why can't we write here just 1024 instead of 1 << 10?

Don't think that getOrElse is a good idea here, as it can hide a possible user error. What logic do you want to follow here? Mb we can add these attributes into the match function above? match supports logical or too.

windowSize is length

pomadchin · 2017-10-10T12:48:10Z

+          val layout = sourceGeoTiffInfo.getGeoTiffInfo(s"s3://$bucket/$key").segmentLayout.tileLayout
+
+          RasterReader
+            .listWindows(cols, rows, options.maxTileSize.getOrElse(1<<10), layout.tileCols, layout.tileRows)


The same is here:

Looks not safe:

options.maxTileSize.getOrElse(1<<10)

Mb to throw an exception or to handle it in a way user will know that smth is used by default (at least to add some warning). In addition, somewhere above you already used getOrElse two times with 1024 default value, mb it makes sense to add these default values somewhere? As we can easily forget about these default values.

Finally the question, mb Options should contain 1024 value for maxTileSize and windowSize by default? (everything is defined in S3GeoTiffRDD object).

As long as changing the default maxTileSize is not construed to constitute an API change, I prefer that.

pomadchin · 2017-10-10T12:54:35Z

    }
  }

+  def windowsByBytes(


Function description is missing, would be good to add.
Btw have you compared it to segmentsByPartitionBytes? Should we remove segmentsByPartitionBytes at all?

pomadchin · 2017-10-10T12:57:57Z

+    (options.maxTileSize, options.partitionBytes) match {
+      case (_, Some(partitionBytes)) => {
+        val windows: RDD[(String, Array[GridBounds])] =
+          sourceGeoTiffInfo.windowsByBytes(partitionBytes, options.maxTileSize.getOrElse(1<<10))


The same comments about maxTileSize and windowSize and 1 << 10 are valid for Hadoop too.

pomadchin · 2017-10-10T13:00:06Z

+    options: Options
+  )(implicit sc: SparkContext, rr: RasterReader[Options, (I, V)]): RDD[(K, V)] = {
+
+    val conf = new SerializableConfiguration(configuration(path, options))


Why conf should be wrapped here? Looks like it's only used for input formats arguments => can be used a common configuration without extra wrapper.

That variable makes its way into a sc.parallelize inside of HadoopGeoTiffInfoReader.

Good point, my bad.

pomadchin · 2017-10-10T13:01:12Z

+  )(implicit sc: SparkContext, rr: RasterReader[Options, (I, V)]): RDD[(K, V)] = {
+
+    val conf = new SerializableConfiguration(configuration(path, options))
+    val path2 = path.toString


Well it's always a bit confusing when you see in the code base such variable names. In fact you can write just: // to avoid variable naming confusion

HadoopGeoTiffInfoReader(path.toString, conf, options.tiffExtensions)

pomadchin · 2017-10-10T13:03:11Z

+          val layout = info.getGeoTiffInfo(objectRequest.toString).segmentLayout.tileLayout
+
+          RasterReader
+            .listWindows(cols, rows, options.maxTileSize.getOrElse(1<<10), layout.tileCols, layout.tileRows)


maxTileSize comment here too

These are given in the respective Options classes to avoid changing semantics.

jamesmcclain · 2017-10-11T17:07:31Z

All comments have been addressed, I believe.

jamesmcclain · 2017-10-11T17:25:59Z

💯

jamesmcclain force-pushed the fix/confirming-windows branch 4 times, most recently from bc6626c to 860cf98 Compare October 4, 2017 12:37

jamesmcclain changed the title ~~[WiP] Generate Windows That Conform GeoTiff Segments~~ [WiP] Generate Windows That Conform To GeoTiff Segments Oct 4, 2017

jamesmcclain force-pushed the fix/confirming-windows branch 3 times, most recently from bb9d4d1 to bccb415 Compare October 5, 2017 02:32

jamesmcclain changed the title ~~[WiP] Generate Windows That Conform To GeoTiff Segments~~ Generate Windows That Conform To GeoTiff Segments Oct 5, 2017

jamesmcclain mentioned this pull request Oct 5, 2017

Filter GeoTiffRDDs by Geometry #2409

Merged

1 task done

echeipesh reviewed Oct 6, 2017

View reviewed changes

echeipesh requested a review from pomadchin October 6, 2017 16:22

pomadchin requested changes Oct 10, 2017

View reviewed changes

James McClain added 14 commits October 11, 2017 12:24

Remove Unnecessary Whitespace

17d5623

Conforming Windows in S3

433ac6d

Conforming Windows in HDFS

354e9c4

Make Windows Conform To Segments, Pack Partitions

2bd1a55

Use windowsByBytes in HDFS

e5e164b

Update and Speed-Up Tests

8c92cee

Respect windowSize in S3

f50e8c5

Use GridBounds.sizeLong

90fd711

Compute Maximum (Linear) Tile Size More Carefully

e50550d

Add DocString To windowsByBytes

f11133f

Remove segmentsByPartitionBytes

d24d708

Compute Maximum (Linear) Tile Size More Carefully

01584ca

Change Variable Name

02a02c7

Supply Last-Choice Tile Sizes

395fe2d

These are given in the respective Options classes to avoid changing semantics.

jamesmcclain force-pushed the fix/confirming-windows branch from 80cdadb to 395fe2d Compare October 11, 2017 16:24

pomadchin approved these changes Oct 11, 2017

View reviewed changes

rossbernet added this to the 1.2 milestone Oct 11, 2017

echeipesh merged commit f2be4df into locationtech:master Oct 11, 2017

echeipesh mentioned this pull request Oct 11, 2017

Partition GeoTiff reading by GeoTiff segment layout #2173

Closed

2 tasks done

jbouffard mentioned this pull request Oct 17, 2017

Tiles Read in Via HadoopGeoTiffRDD or S3GeoTiffRDD Should be the Specified Size #2435

Closed

pomadchin mentioned this pull request Nov 3, 2017

Ingesting large geotiff from s3 doesn't seem to be partitioning data properly #2469

Closed

jamesmcclain deleted the fix/confirming-windows branch November 17, 2017 21:14

pomadchin mentioned this pull request Dec 19, 2018

HadoopGeoTiffRDD (at least) reads segments multiple times #2299

Closed

Conversation

jamesmcclain commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echeipesh commented Oct 6, 2017

Uh oh!

jamesmcclain commented Oct 6, 2017

Uh oh!

pomadchin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesmcclain commented Oct 11, 2017

Uh oh!

jamesmcclain commented Oct 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamesmcclain commented Oct 2, 2017 •

edited

Loading

pomadchin Oct 10, 2017 •

edited

Loading

pomadchin Oct 10, 2017 •

edited

Loading