Expose GDAL Metadata API, expose RasterSources Metadata API #216

pomadchin · 2019-07-18T16:59:47Z

Overview

This PR exposes the metadata that is not covered by the RasterSources API, such as tiff tags / extra metadata that can be interpreted as a string.

In terms of this PR we've tried to look how other GIS libraries collect imagery metadata, and it looks like there is no a standard interface to work with rather than a Map[String, String].

Checklist

Add entry to CHANGELOG.md

Closes #181

vlm/src/main/scala/geotrellis/contrib/vlm/RasterSourceMetadata.scala

vlm/src/main/scala/geotrellis/contrib/vlm/SourceMetadata.scala

pomadchin · 2019-07-29T22:03:37Z

vlm/src/main/scala/geotrellis/contrib/vlm/RasterSource.scala

@@ -42,41 +42,11 @@ import java.util.ServiceLoader
  * @groupdesc reproject Functions to resample raster data in target projection.
  * @groupprio reproject 2
  */
-trait RasterSource extends CellGrid[Long] with Serializable {
+trait RasterSource extends CellGrid[Long] with RasterSourceMetadata with Serializable {


Does this mixin makes any sense? CellGrid[Long] with RasterSourceMetadata

Good question.... what is gained by having it subclass CellGrid[Long]. Should RasterSourceMetadata be the one to extend it? At least at a superficial level, both feel a bit like responsibility leakage into RasterSource.

There's an argument that the relationship between RasterSource and RasterSourceMetadata should be a "has a" relationship and not an "is a" one. I tend to lean toward smaller, multiply nested types (more "has a"), but that can get annoying to traverse. If I were implementing this I would probably have RasterSource extend neither (or keep CellGrid if it's already depended upon), and then have a method metadata: RasterSourceMetadata that the user hits to get the deets. And then add convenience forwarding methods as needed for a subset of fields such as crs, bandCount, etc. It's an approach emphasizing separation of concerns over convenience, which, like anything, can be taken too far. I think my main argument would be that it's easier to add things later than to take them away if the abstraction isn't quite right.

pomadchin · 2019-07-30T17:57:07Z

@echeipesh I am not sure does the commit pomadchin@ec533ea makes things better or not :/ need a pair of fresh eyes here.

I even decided to roll it back, since I don't like it. However I put it into a separate branch for the history / review purposes.

vlm/src/main/scala/geotrellis/contrib/vlm/SourceMetadata.scala

vlm/src/main/scala/geotrellis/contrib/vlm/avro/GeoTrellisMetadata.scala

vlm/src/test/scala/geotrellis/contrib/vlm/avro/GeotrellisRasterSourceSpec.scala

metasim · 2019-07-31T13:04:26Z

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALDataset.scala

 import com.azavea.gdal.GDALWarp

 case class GDALDataset(token: Long) extends AnyVal {
+  def getAllMetadataFlatten(dataset: Int): Map[String, String] =


metasim · 2019-07-31T13:05:53Z

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALDataset.scala

+
+  /** https://github.com/OSGeo/gdal/blob/b1c9c12ad373e40b955162b45d704070d4ebf7b0/gdal/doc/source/development/rfc/rfc43_getmetadatadomainlist.rst */
+  def getMetadataDomainList(dataset: Int, band: Int): List[String] = {
+    val arr = Array.ofDim[Byte](100, 1 << 10)


As I said before, I think this is half the cause of the performance issues (triggering lots of copies to happen around the JNI boundary). Hoping this signature is considered.

@metasim I need to benchmark @jamesmcclain bindings; I think it is a completely separate issue now, as probably requires some investigation time

metasim · 2019-07-31T13:07:41Z

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALDataset.scala

+
+    new String(arr, "UTF-8").trim
+  }
+
  def getProjection: Option[String] = getProjection(GDALWarp.WARPED)


Can you document something about the (expected) returned string format? If it's guaranteed to be a well-formed/parseable structure, should we type-tag it?

Or mb even opaque types (https://github.com/estatico/scala-newtype); but it is not a part of this PR

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALDataset.scala

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALMetadata.scala

metasim · 2019-07-31T13:18:53Z

vlm/src/main/scala/geotrellis/contrib/vlm/RasterSource.scala

@@ -42,41 +42,11 @@ import java.util.ServiceLoader
  * @groupdesc reproject Functions to resample raster data in target projection.
  * @groupprio reproject 2
  */
-trait RasterSource extends CellGrid[Long] with Serializable {
+trait RasterSource extends CellGrid[Long] with RasterSourceMetadata with Serializable {


Good question.... what is gained by having it subclass CellGrid[Long]. Should RasterSourceMetadata be the one to extend it? At least at a superficial level, both feel a bit like responsibility leakage into RasterSource.

There's an argument that the relationship between RasterSource and RasterSourceMetadata should be a "has a" relationship and not an "is a" one. I tend to lean toward smaller, multiply nested types (more "has a"), but that can get annoying to traverse. If I were implementing this I would probably have RasterSource extend neither (or keep CellGrid if it's already depended upon), and then have a method metadata: RasterSourceMetadata that the user hits to get the deets. And then add convenience forwarding methods as needed for a subset of fields such as crs, bandCount, etc. It's an approach emphasizing separation of concerns over convenience, which, like anything, can be taken too far. I think my main argument would be that it's easier to add things later than to take them away if the abstraction isn't quite right.

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALMetadata.scala

pomadchin · 2019-07-31T20:02:54Z

gdal/src/main/scala/geotrellis/contrib/vlm/gdal/GDALMetadata.scala

+          dataset.getMetadata(GDALWarp.SOURCE, domains, 0),
+          (1 until dataset.bandCount).toList.map(dataset.getMetadata(GDALWarp.SOURCE, domains, _))
+        )
+      case FullDomainList =>


Mb it should be AllMetadataDomainsList? Or smth like that.

pomadchin · 2019-07-31T20:38:12Z

Travis is down for some reason :/ in my fork it passes tests https://travis-ci.org/pomadchin/geotrellis-contrib/jobs/566145813

…etadata collection, simplify the metadata concept.

echeipesh

💯 after GDALWarp.WARPED becomes an ADT and GDALMetadataDomainList gets deleted in favor of GDALRasterSource.metadataForAllDomains

pomadchin force-pushed the feature/rs-effects-metadata branch 3 times, most recently from b1b3f85 to 3620ca4 Compare July 18, 2019 18:46

pomadchin mentioned this pull request Jul 23, 2019

RasterSource Metadata API #181

Closed

metasim reviewed Jul 23, 2019

View reviewed changes

pomadchin mentioned this pull request Jul 24, 2019

LayoutTileSource[K] implementation #220

Merged

1 task

pomadchin force-pushed the feature/rs-effects-metadata branch from 3620ca4 to fe6a5ba Compare July 29, 2019 15:53

pomadchin added 2 commits July 29, 2019 16:42

Expose GDAL Metadata API, expose RasterSources Metadata API

2bfc881

Add a MosaicMetadata, remove the combine method definition

0cb1d59

pomadchin force-pushed the feature/rs-effects-metadata branch from 3e3fb55 to 0cef544 Compare July 29, 2019 21:54

pomadchin requested a review from echeipesh July 29, 2019 22:00

pomadchin force-pushed the feature/rs-effects-metadata branch from 0cef544 to c3f4a5d Compare July 29, 2019 22:02

pomadchin commented Jul 29, 2019

View reviewed changes

pomadchin force-pushed the feature/rs-effects-metadata branch 2 times, most recently from 3daef0d to e4c4db6 Compare July 30, 2019 17:54

pomadchin commented Jul 30, 2019

View reviewed changes

vlm/src/main/scala/geotrellis/contrib/vlm/SourceMetadata.scala Outdated Show resolved Hide resolved

pomadchin commented Jul 30, 2019

View reviewed changes

vlm/src/main/scala/geotrellis/contrib/vlm/avro/GeoTrellisMetadata.scala Outdated Show resolved Hide resolved

pomadchin force-pushed the feature/rs-effects-metadata branch from e4c4db6 to 69b5562 Compare July 30, 2019 18:36

pomadchin commented Jul 30, 2019

View reviewed changes

vlm/src/test/scala/geotrellis/contrib/vlm/avro/GeotrellisRasterSourceSpec.scala Outdated Show resolved Hide resolved

pomadchin force-pushed the feature/rs-effects-metadata branch 2 times, most recently from ec533ea to cffb32a Compare July 30, 2019 20:51

Refactor metadata classes

783068b

pomadchin force-pushed the feature/rs-effects-metadata branch from cffb32a to 783068b Compare July 30, 2019 20:54

metasim reviewed Jul 31, 2019

View reviewed changes

pomadchin force-pushed the feature/rs-effects-metadata branch 4 times, most recently from f31ddd5 to ce1e404 Compare July 31, 2019 19:45

pomadchin requested a review from CloudNiner July 31, 2019 19:59

pomadchin commented Jul 31, 2019

View reviewed changes

Reflect review changes: change methods and traits name and logic of m…

a93ab9e

…etadata collection, simplify the metadata concept.

pomadchin force-pushed the feature/rs-effects-metadata branch from ce1e404 to a93ab9e Compare July 31, 2019 22:50

echeipesh approved these changes Jul 31, 2019

View reviewed changes

pomadchin force-pushed the feature/rs-effects-metadata branch from 49978c4 to 35b1167 Compare July 31, 2019 23:34

Simplify GDALMetadata building, make GDALDatasetType an ADT

fade0f4

pomadchin force-pushed the feature/rs-effects-metadata branch from 35b1167 to fade0f4 Compare July 31, 2019 23:40

echeipesh merged commit 81f7c2d into geotrellis:master Aug 1, 2019

This was referenced Aug 1, 2019

Add support for Tags in RasterSource #143

Closed

GDALRasterSource Metadata #99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose GDAL Metadata API, expose RasterSources Metadata API #216

Expose GDAL Metadata API, expose RasterSources Metadata API #216

pomadchin commented Jul 18, 2019 •

edited

Loading

pomadchin Jul 29, 2019

metasim Jul 31, 2019

pomadchin commented Jul 30, 2019 •

edited

Loading

metasim Jul 31, 2019

metasim Jul 31, 2019

pomadchin Jul 31, 2019

metasim Jul 31, 2019

pomadchin Jul 31, 2019

metasim Jul 31, 2019

pomadchin Jul 31, 2019

pomadchin commented Jul 31, 2019

echeipesh left a comment

Expose GDAL Metadata API, expose RasterSources Metadata API #216

Expose GDAL Metadata API, expose RasterSources Metadata API #216

Conversation

pomadchin commented Jul 18, 2019 • edited Loading

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomadchin commented Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pomadchin commented Jul 31, 2019

echeipesh left a comment

Choose a reason for hiding this comment

pomadchin commented Jul 18, 2019 •

edited

Loading

pomadchin commented Jul 30, 2019 •

edited

Loading