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

GeoTrellisRasterSource temporal build improvements #219

Closed
pomadchin opened this issue Feb 25, 2020 · 5 comments · Fixed by #234
Closed

GeoTrellisRasterSource temporal build improvements #219

pomadchin opened this issue Feb 25, 2020 · 5 comments · Fixed by #234
Assignees

Comments

@pomadchin
Copy link
Member

pomadchin commented Feb 25, 2020

At this point GeoTrellisRasterSource.build function creates a set of GeoTrellisRasterSources from a temporal layer, making an assumption that for each time bin there is a single spatial slice. This is not true in the most cases in real world:

  1. Instead of an optional temporal resolution we need to pass a list of dates that we know are present in the layer,
  2. Have an optional ability to read the list of dates that correspond to each temporal slice from the AttributeStore (this can be done as some custom attribute saved into the AttributeStore).
  3. Creation of each new GeoTrellisRasterSource for each date is expensive now, since for each new instance we read all the metadata everytime for each slice, though it is the same across all of them.
@echeipesh
Copy link
Collaborator

Fresh after reviewing the Query DSL PR these are my notes on what the work probably should be, take it for FWIW:

  • We need to apply DSL on something other than List[RasterSource] because premature creation of RasterSource instances is potentially expensive.
  • We can use List[OgcSourceConf] since that's what we're reading out anyway.
  • OgcSourceConf should be extended to provide list of dates for the file.
  • For OgcSourceConf based on GT RasterSource these should be read from times attribute in the AttributeStore for zoom level 0.
  • We should expect time in ISO 8601 format

@CloudNiner
Copy link
Contributor

I'm releasing my assignment on this as it wasn't strictly necessary in the implementation of #223. Going to move onto other stuff first and pull this again if it becomes a bottleneck.

@CloudNiner CloudNiner removed their assignment Feb 27, 2020
@CloudNiner
Copy link
Contributor

A small SpaceTimeKey layer using landsat 8 data is available at s3://geotrellis-test-non-public/spacetimekey-test, layer name spacetimekey-test, zoom 14. I generated it with the code here as part of #157.

@CloudNiner
Copy link
Contributor

Copying these "where to start" notes from a conversation with Grisha just now:

  1. https://github.com/geotrellis/geotrellis-server/blob/develop/core/src/main/scala/geotrellis/store/GeoTrellisRasterSourceLegacy.scala#L283-L289 temporal resolution is incorrect the fist thing to change is that we need to persist into the attribute store (as a custom field) a list of times that can be selected from the temporal layer // that is acutally what was Eugene talking about
  2. https://github.com/geotrellis/geotrellis-server/blob/develop/core/src/main/scala/geotrellis/store/GeoTrellisRasterSourceLegacy.scala#L294 - it is extremely expensive to create a bucnh of GTRasterSources - we can read all the metadata once and pass it to all rastersource through the constructor (if we can not - fix it so we can ):)
  3. it is about List of RasterSources; I don’t know what the return type of a build function should be, but i know that
    def toRasterSources: List[RasterSource] =
    GeoTrellisRasterSourceLegacy.build(GeoTrellisPath(catalogUri, layer, Some(zoom), Some(bandCount)))
    }
    /** A geotiff (COG) raster source */
    case class GeoTiff(uri: String) extends RasterSourceConf {
    def toRasterSources: List[GeoTiffRasterSource] = GeoTiffRasterSource(uri) :: Nil
    this had to return a single RasterSource (but you’ve merged your commit and thigs have chnaged a bit)

@echeipesh
Copy link
Collaborator

Just a note, my interpretation is that at the end of this PR its going to be clear how to delete GeoTrellisRasterSourceLegacy and use the main-line version of that class. Doing that should be a follow-up issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants