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

readSubsetBands for COGValueReader and OverzoomingCOGValueReader #2757

Merged
merged 3 commits into from Jul 11, 2018

Conversation

jbouffard
Copy link

@jbouffard jbouffard commented Jul 6, 2018

Overview

This PR adds the readSubsetBands method to COGValueReader and OverzoomingCOGValueReader. This method will allow users to read in only certain bands from a COG layer in whatever order they want.

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

Demo

val reader: COGReader[SpatialKey, MultibandTile] = ???
val key: SpatialKey = ???

// Tries to read bands 2, 0, 1, 10 and return them in that order
reader.readSubsetBands(key, 2, 0, 1, 10)

Related to #2706

@jbouffard jbouffard force-pushed the feature/cog-band-subsetting branch from 671a22b to 25f626f Compare July 6, 2018 17:14
@jbouffard jbouffard changed the title Feature/cog band subsetting readSubsetBands for COGValueReader and OverzoomingCOGValueReader Jul 6, 2018
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.

Hm, looks pretty good. Not sure if we can reduce the code amount in terms of this PR as metadata changes can be a different issue. I left a couple comments though.

.toArray

val targetBands: Array[Int] = targetBandsWithIndex.map { _._1 }
val targetBandsIndexes: Array[Int] = targetBandsWithIndex.map { _._2 }
Copy link
Member

Choose a reason for hiding this comment

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

You can make it in one command:

val (targetBands, targetBandsIndexes) = targetBandsWithIndex.unzip

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I didn't know about that method. That does make things easier to read

// Have to convert the sourceGeoTiff to a GeoTiffMultibandTile
// in order to access the desired crop method.
val sourceMultibandTile =
GeoTiffMultibandTile(
Copy link
Member

Choose a reason for hiding this comment

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

I suppose It's possible to use pattern matching here, but this solution is also good.


}

def readSubsetBands(key: K, bands: Seq[Int]): Array[Option[Tile]] = {
Copy link
Member

Choose a reason for hiding this comment

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

wondering about how we can express read using this readSubsetBands method.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking something like:

def read(key: K, bands: Option[Seq[Int]] = None): K

Where None specifies all bands to be read in. That's still a rough idea, though, so I'd actually need to see if it can be implemented better.

Copy link
Member

@pomadchin pomadchin Jul 10, 2018

Choose a reason for hiding this comment

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

Yea, I'm thinking that probably there should be an overload that would convert this Array[Option[Tile]] into V type. It can be useful in cases people want to manage their bands numbers themselves, it would work similar to MultibandGeoTiffTile type.

@@ -0,0 +1 @@
[{"name":"multiband-cog-layer","zoom":0},{"header":{"format":"file","path":"spark/src/test/resources/cog-layer","layerType":"COGLayerType","keyClass":"geotrellis.spark.SpatialKey","valueClass":"geotrellis.raster.MultibandTile"},"metadata":{"metadata":{"zoomRangesInfos":[[{"minZoom":0,"maxZoom":0},{"minKey":{"col":0,"row":0},"maxKey":{"col":0,"row":0}}],[{"minZoom":1,"maxZoom":1},{"minKey":{"col":1,"row":0},"maxKey":{"col":1,"row":0}}],[{"minZoom":2,"maxZoom":2},{"minKey":{"col":2,"row":1},"maxKey":{"col":2,"row":1}}],[{"minZoom":3,"maxZoom":3},{"minKey":{"col":5,"row":3},"maxKey":{"col":5,"row":3}}],[{"minZoom":4,"maxZoom":4},{"minKey":{"col":11,"row":7},"maxKey":{"col":11,"row":7}}],[{"minZoom":5,"maxZoom":5},{"minKey":{"col":23,"row":14},"maxKey":{"col":23,"row":15}}],[{"minZoom":6,"maxZoom":6},{"minKey":{"col":46,"row":29},"maxKey":{"col":46,"row":30}}],[{"minZoom":7,"maxZoom":7},{"minKey":{"col":92,"row":59},"maxKey":{"col":92,"row":60}}],[{"minZoom":8,"maxZoom":11},{"minKey":{"col":184,"row":118},"maxKey":{"col":185,"row":120}}]],"extent":{"xmin":80.0,"ymin":5.0,"xmax":81.0,"ymax":7.0},"layoutScheme":{"crs":"+proj=longlat +datum=WGS84 +no_defs ","tileSize":256,"resolutionThreshold":0.1},"cellType":"int16","crs":"+proj=longlat +datum=WGS84 +no_defs "},"keyIndexes":[[{"minZoom":0,"maxZoom":0},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":0,"row":0},"maxKey":{"col":0,"row":0}}}}],[{"minZoom":7,"maxZoom":7},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":92,"row":59},"maxKey":{"col":92,"row":60}}}}],[{"minZoom":6,"maxZoom":6},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":46,"row":29},"maxKey":{"col":46,"row":30}}}}],[{"minZoom":4,"maxZoom":4},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":11,"row":7},"maxKey":{"col":11,"row":7}}}}],[{"minZoom":8,"maxZoom":11},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":184,"row":118},"maxKey":{"col":185,"row":120}}}}],[{"minZoom":1,"maxZoom":1},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":1,"row":0},"maxKey":{"col":1,"row":0}}}}],[{"minZoom":2,"maxZoom":2},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":2,"row":1},"maxKey":{"col":2,"row":1}}}}],[{"minZoom":5,"maxZoom":5},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":23,"row":14},"maxKey":{"col":23,"row":15}}}}],[{"minZoom":3,"maxZoom":3},{"type":"zorder","properties":{"keyBounds":{"minKey":{"col":5,"row":3},"maxKey":{"col":5,"row":3}}}}]]}}]
Copy link
Member

Choose a reason for hiding this comment

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

So you decided not to zip this layer?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, but only because that's what we seemed to be doing with all of the other test files in that directory

@jbouffard jbouffard force-pushed the feature/cog-band-subsetting branch from 25f626f to 30f9c4e Compare July 10, 2018 12:50
val iter = crop(List(bounds), bandIndices)
if (iter.isEmpty) throw GeoAttrsError(s"No intersections of ${bounds} vs ${gridBounds}")
else iter.next._2
}
Copy link
Member

Choose a reason for hiding this comment

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

We can DRY out this code, by having the above crop call this overload. That's the preferred pattern through the library.

@@ -45,16 +45,39 @@ trait OverzoomingCOGValueReader extends COGValueReader[LayerId] {
lazy val baseReader = reader[K, V](layerId)
lazy val maxReader = reader[K, V](LayerId(layerName, maxAvailableZoom))

def read(key: K): V =
private def deriveMaxKey(key: K): K = {
Copy link
Member

Choose a reason for hiding this comment

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

no need for private here, it's inside a method

exp should be (act)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Are we testing both pixel and band interleave tiles?

Copy link
Author

Choose a reason for hiding this comment

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

That is more of general test to make sure that it works. I believe the crop logic has been tested with both band and pixel interleave though.

Jacob Bouffard added 3 commits July 10, 2018 12:15
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Updated the CHANGELOG with the new crop method

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Cleaned up the crop method

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
OverzoomingCOGValueReader

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

COGValueReader now produces a COGReader

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed the logic in the readSubsetBands method

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed the logic some more in readSubsetBands

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed bug in S3COGValueReader

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Added a readSubsetBands test in COGFileSpatialSpec

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Created the multiband-cog-layer test data

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Added the readSubsetBands method to OverzoomingCOGValueReader

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Updated the CHANGELOG with the readSubsetBands method

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Cleaned up readSubsetBands some more

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Made deriveMaxKey not private

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
@jbouffard jbouffard force-pushed the feature/cog-band-subsetting branch from 30f9c4e to 9b01c89 Compare July 10, 2018 16:15
@pomadchin pomadchin merged commit 0af8404 into locationtech:master Jul 11, 2018
@echeipesh echeipesh added this to the 2.0 milestone Aug 3, 2018
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.

None yet

4 participants