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.

Sign up for GitHub

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

Jump to bottom

LayoutTileSource[K] implementation #220

Merged
merged 5 commits into from
Aug 8, 2019
Merged

LayoutTileSource[K] implementation #220

merged 5 commits into from
Aug 8, 2019

Conversation

pomadchin
Copy link
Member

@pomadchin pomadchin commented Jul 24, 2019 •
edited
Loading

Overview

This PR adds an ability to work with temporal keys and RasterSources

Checklist

  • Add entry to CHANGELOG.md

Closes #182

@pomadchin pomadchin force-pushed the feature/temporal-api branch from 70a238a to 6556cc2 Compare July 24, 2019 01:04
@pomadchin
Copy link
Member Author

This is a first attempt to resolve the problem of extending the API to support temporal keys.

@pomadchin pomadchin force-pushed the feature/temporal-api branch from 6556cc2 to 35b9899 Compare July 24, 2019 01:08
@pomadchin pomadchin force-pushed the feature/temporal-api branch from 35b9899 to a09736e Compare July 24, 2019 01:36
@pomadchin pomadchin changed the title LayoutTileSource[K] first implementation LayoutTileSource[K] implementation Jul 24, 2019
@pomadchin pomadchin force-pushed the feature/temporal-api branch from a09736e to ec392e3 Compare July 24, 2019 02:53
@pomadchin pomadchin force-pushed the feature/temporal-api branch 2 times, most recently from c8b35e0 to b32f766 Compare July 24, 2019 13:08
@CloudNiner CloudNiner mentioned this pull request Jul 25, 2019
Closed
@echeipesh echeipesh self-requested a review July 25, 2019 19:52
@pomadchin pomadchin force-pushed the feature/temporal-api branch 3 times, most recently from 4e99506 to 7d7b125 Compare July 31, 2019 21:24
@pomadchin pomadchin force-pushed the feature/temporal-api branch from 7d7b125 to 55355e1 Compare August 1, 2019 11:49
@pomadchin pomadchin force-pushed the feature/temporal-api branch 2 times, most recently from 291981e to 867f598 Compare August 3, 2019 16:22
@pomadchin pomadchin force-pushed the feature/temporal-api branch 2 times, most recently from d0d4e76 to 5f15b16 Compare August 3, 2019 16:28
pomadchin
pomadchin commented Aug 3, 2019
View reviewed changes
vlm/src/main/scala/geotrellis/contrib/vlm/TileToLayout.scala Outdated
* In order to fit to the given layout, the source data is resampled to match the Extent
* and CellSize of the layout.
*/
trait TileToLayout[K] extends Serializable {
Copy link
Member Author

Choose a reason for hiding this comment

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

TileToLayout has to be a type class to choose the instance in compile time depending on the Key type.

@pomadchin pomadchin force-pushed the feature/temporal-api branch from 5f15b16 to 787c823 Compare August 3, 2019 16:31
pomadchin
pomadchin commented Aug 3, 2019
View reviewed changes
vlm/src/main/scala/geotrellis/contrib/vlm/spark/RasterSourceRDD.scala

val cellType = cellTypes.head
val crs = projections.head
def read(
Copy link
Member Author

Choose a reason for hiding this comment

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

Added lots of fancy overloads to hide type parameters from places where they are not necessary.

pomadchin
pomadchin commented Aug 3, 2019
View reviewed changes
vlm/src/main/scala/geotrellis/contrib/vlm/spark/RasterSummary.scala Outdated
)
}
}

object RasterSummary {
/** Collect [[RasterSummary]] from unstructred rasters, grouped by CRS */
def collect[V <: CellGrid[N]: GetComponent[?, ProjectedExtent], N: Integral](rdd: RDD[V]): Seq[RasterSummary] = {
def collect[K: SpatialComponent: Boundable](rdd: RDD[RasterSource], keyTransform: (RasterSource, SpatialKey) => K): Seq[RasterSummary[K]] = {
Copy link
Member Author

Choose a reason for hiding this comment

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

It still has to be a RasterSource and not it's metadata, as we don't pass the tile path as a part of the metadata information. I think it will be resolved in terms of #197 as here we have to decide what to do with path and what if we want to give some name to the raster source that is different from the path (to follow the GDALDataset names logic).

pomadchin
pomadchin commented Aug 3, 2019
View reviewed changes
vlm/src/test/scala/geotrellis/contrib/vlm/spark/RasterSummarySpec.scala Outdated
})

// collect raster summary
val summary = RasterSummary.fromRDD(sourceRDD, keyTransform)
Copy link
Member Author

Choose a reason for hiding this comment

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

To collect RasterSummary[SpatialKey] we can use RasterSummary.fromRDD(sourceRDD), for temporal and other K types we can just add a keyTransform function definition.

@pomadchin
Copy link
Member Author

pomadchin commented Aug 3, 2019 •
edited
Loading

This PR tries to minimize global changes in the code as we want to include these changes into the release. The core problem is the metadata collection step, and the way it is done now is a little bit complicated (talking about the API ~ as it is pretty hard to explain to users that they need to collect some RasterSummary to derive a LayoutDefintion).

Much more sense makes to have some sort of a MosaicRasterSource but for RDDs (or abstracted over the collection type) that will incapsulate the metadata collection logic. However, it may require some extra time to think about it and to design it, and at this point probably it makes sense just to modify the existing metadata collection to take into account gathering any information from the RasterSource to build some user defined K. Also it definitely sounds like a separate issue.

@pomadchin pomadchin self-assigned this Aug 3, 2019
@pomadchin pomadchin force-pushed the feature/temporal-api branch from 787c823 to 008748c Compare August 3, 2019 16:53
@pomadchin pomadchin force-pushed the feature/temporal-api branch from 008748c to 453df75 Compare August 3, 2019 18:07
@pomadchin pomadchin force-pushed the feature/temporal-api branch 2 times, most recently from 669d41f to bdd8bfd Compare August 7, 2019 23:19
@pomadchin pomadchin force-pushed the feature/temporal-api branch from bdd8bfd to fc45c29 Compare August 7, 2019 23:31
echeipesh
echeipesh reviewed Aug 7, 2019
View reviewed changes
vlm/src/main/scala/geotrellis/contrib/vlm/KeyExtractor.scala
}
}

trait TemporalKeyExtractor extends KeyExtractor[SpaceTimeKey] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For when this code moves around: TemporalKeyExtractor and spatialKeyExtractor should both be classes for members of KeyExtractor companion object to maximize uniformity and minimize surprise.

@echeipesh echeipesh merged commit 07a5955 into geotrellis:master Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@echeipesh echeipesh echeipesh approved these changes

Assignees

@pomadchin pomadchin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RasterSource can be used to produce MultibandTileLayerRDD[SpaceTimeKey]
2 participants
@pomadchin @echeipesh