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

Add ETL option to specify upper zoom limit for raster layer ingestion #1494

Merged
merged 4 commits into from Jul 7, 2016

Conversation

Projects
None yet
3 participants
@mbertrand
Contributor

mbertrand commented May 23, 2016

This PR addresses #1484 by adding an optional "maxZoom" argument to ETLConf (with a default value of 0), and modifies both the Etl and TileLayerMetadata classes to handle that option.

In testing with the Chattanooga demo however, setting maxZoom levels above 10 often led to the ingestion process being killed due to lack of memory. Increasing VM memory to 6 GB, and the memory available to the ingestion process to 4 GB, allowed the ingestion process to succeed up to zoom level 12.

val (extent, cellType, cellSize, bounds, crs) = collectMetadataWithCRS(rdd)
val LayoutLevel(zoom, layout) = scheme.levelFor(extent, cellSize)
val LayoutLevel(zoom, layout) = (if (maxZoom > 0) scheme.asInstanceOf[ZoomedLayoutScheme].levelForZoom(maxZoom)

This comment has been minimized.

@lossyrob

lossyrob May 23, 2016

Member

Probably should use an Option[Int] type for the maxZoom, so if someone calls the argument with a maxZoom of 0, we can deal with that error differently than if the argument was not there at all.

We wouldn't want to put this asInstanceOf in here (asInstanceOf is considered Evil in Scala and we should only use it as a last resort).

The maxZoom parameter shouldn't be used with anything but a tms layout, which could be detected in the EtlConfig code and an error would be issued at that point.

Here we could avoid the asInstanceOf by matching

val LayoutLevel(zoom, layout) = {
  maxZoom match {
    case Some(zoom) =>
      scheme match {
         case zoomedLayoutScheme: ZoomedLayoutScheme =>
            zoomedLayoutScheme.levelFor(extent, cellSize)
         case _ => // throw a descriptive error
      }
    case None =>
      scheme.levelFor(extent, cellSize)
  }
}

This comment has been minimized.

@mbertrand

mbertrand May 24, 2016

Contributor

@lossyrob Thanks for the feedback and sorry for the evil Scala! I've updated the code.

case zoomedLayoutScheme: ZoomedLayoutScheme => logger.info("maxZoom and layout scheme compatible")
case _ => throw new RuntimeException("maxZoom option should only be used with tms layout scheme")
}
}

This comment has been minimized.

@lossyrob

lossyrob May 24, 2016

Member

This works, but I'm wondering if there's perhaps a better way to handle the conflict.

@echeipesh you would know, can you suggest another way or else we can just keep this as is.

This comment has been minimized.

@echeipesh

echeipesh May 25, 2016

Contributor

I would suggest to move the validation to EtlConf object and use the validateOpt helper from scallop: https://github.com/scallop/scallop/wiki/Arguments-validation#custom-validation

This comment has been minimized.

@mbertrand

mbertrand May 26, 2016

Contributor

Thanks @echeipesh,
I've been trying to implement this but can't get it to work properly, I think because layoutScheme is returning a LayoutSchemaProvider instead of a LayoutScheme (Zoomed or Floating) within the validate function. Any ideas on how to get the actual LayoutScheme class here?

  validateOpt (layoutScheme, maxZoom) {
    case (Some(schema), Some(zoom)) =>  schema match {
        case scheme: ZoomedLayoutScheme => Right(Unit)
        case _ => Left("maxZoom should only be used with ZoomedLayoutScheme (tms)")
      }
    case (None, Some(zoom)) => Left("maxZoom not be used when a LayoutScheme is not provided")
    case _ => Right(Unit)
  }

This comment has been minimized.

@echeipesh

echeipesh May 26, 2016

Contributor

Ah, that's a good point. That makes it more difficult than it should be. I think rather than trying to get fancy with config parsing lets stick with your original approach.

scheme match {
case zoomedLayoutScheme: ZoomedLayoutScheme =>
zoomedLayoutScheme.levelForZoom(maxZoom.get)
case _ => throw new RuntimeException("ZoomedLayoutScheme required when setting a max zoom level")

This comment has been minimized.

@lossyrob

lossyrob May 24, 2016

Member

Instead of throwing here, I would like to enforce this with the type system. This is a situation where I would prefer some code duplication for type safety. So keeping the original fromRDD's, and adding

def fromRdd[
    K: GetComponent[?, ProjectedExtent]: (? => TilerKeyMethods[K, K2]),
    V <: CellGrid,
    K2: SpatialComponent: Boundable
  ](rdd: RDD[(K, V)], scheme: ZoomedLayoutScheme, maxZoom: Int): (Int, TileLayerMetadata[K2]) = {
    val (extent, cellType, cellSize, bounds, crs) = collectMetadataWithCRS(rdd)
    val LayoutLevel(zoom, layout) =
      scheme.levelForZoom(maxZoom)

    val GridBounds(colMin, rowMin, colMax, rowMax) = layout.mapTransform(extent)
    val kb = bounds.setSpatialBounds(KeyBounds(layout.mapTransform(extent)))
    (zoom, TileLayerMetadata(cellType, layout, extent, crs, kb))
  }

(and one for the other type of fromRdd call).

@lossyrob

This comment has been minimized.

Member

lossyrob commented May 24, 2016

This is looking good! Just added some more comments.

Before we merge these changes, you'll have to sign an Eclipse CLA. Can you sign one with an eclipse account tied to the same email address you commit to github with, and let me know when you have that? You can follow the right hand side instructions of here: https://www.eclipse.org/legal/CLA.php

Thanks!

@mbertrand

This comment has been minimized.

Contributor

mbertrand commented May 25, 2016

Thanks @lossyrob, I made some more updates to the code and signed the Eclipse CLA.

@echeipesh

This comment has been minimized.

Contributor

echeipesh commented Jun 6, 2016

@mbertrand I am failing to verify the Eclipse CLA through: https://projects.eclipse.org/user/cla/validate
I would guess you used an email other than the one attached to these commits.

K: (? => TilerKeyMethods[K, K2]) ,
V <: CellGrid,
K2: SpatialComponent: Boundable
](rdd: RDD[(K, V)], crs: CRS, scheme: ZoomedLayoutScheme, maxZoom: Option[Int] = None):

This comment has been minimized.

@lossyrob

lossyrob Jun 7, 2016

Member

Unfortunately Scala does not allow default arguments on a method that is overloaded. The rules around this are strange, and sometimes the compiler does not catch this by default. I think it might only happen on 2.11, but if you run these commands, you'll get these outputs:

./sbt
geotrellis > project spark
spark > +package
[info] Compiling 230 Scala sources to /Users/rob/proj/gt/geotrellis-codereview/spark/target/scala-2.11/classes...
[error] /Users/rob/proj/gt/geotrellis-codereview/spark/src/main/scala/geotrellis/spark/TileLayerMetadata.scala:66: in object TileLayerMetadata, multiple overloaded alternatives of method fromRdd define default arguments.
[error] object TileLayerMetadata {
[error]        ^
[error] one error found
[error] (spark/compile:compileIncremental) Compilation failed
[error] Total time: 75 s, completed Jun 7, 2016 11:30:43 AM
spark > 

That means we have to create overloads for the default options. And since we have to do that anyway, might be good to not require the option

e.g.

  def fromRdd[
    K: (? => TilerKeyMethods[K, K2]) ,
    V <: CellGrid,
    K2: SpatialComponent: Boundable
  ](rdd: RDD[(K, V)], crs: CRS, scheme: ZoomedLayoutScheme, maxZoom: Option[Int] = None):
  (Int, TileLayerMetadata[K2]) = {
    val (extent, cellType, cellSize, bounds) = collectMetadata(rdd)
    val LayoutLevel(zoom, layout) = maxZoom match {
      case Some(zoom) => scheme.levelForZoom(maxZoom.get)
      case _ => scheme.levelFor(extent, cellSize)
    }
    val kb = bounds.setSpatialBounds(KeyBounds(layout.mapTransform(extent)))
    (zoom, TileLayerMetadata(cellType, layout, extent, crs, kb))
  }

becomes something like

def fromRdd[
    K: (? => TilerKeyMethods[K, K2]) ,
    V <: CellGrid,
    K2: SpatialComponent: Boundable
  ](rdd: RDD[(K, V)], crs: CRS, scheme: ZoomedLayoutScheme):
  (Int, TileLayerMetadata[K2]) =
   _fromRdd[K, V, K2](rdd, crs, scheme, None)

 def fromRdd[
    K: (? => TilerKeyMethods[K, K2]) ,
    V <: CellGrid,
    K2: SpatialComponent: Boundable
  ](rdd: RDD[(K, V)], crs: CRS, scheme: ZoomedLayoutScheme, maxZoom: Int):
  (Int, TileLayerMetadata[K2]) =
   _fromRdd[K, V, K2](rdd, crs, scheme, Some(maxZoom))

 private def _fromRdd[
    K: (? => TilerKeyMethods[K, K2]) ,
    V <: CellGrid,
    K2: SpatialComponent: Boundable
  ](rdd: RDD[(K, V)], crs: CRS, scheme: ZoomedLayoutScheme, maxZoom: Option[Int]):
  (Int, TileLayerMetadata[K2]) = {
    val (extent, cellType, cellSize, bounds) = collectMetadata(rdd)
    val LayoutLevel(zoom, layout) = maxZoom match {
      case Some(zoom) => scheme.levelForZoom(maxZoom.get)
      case _ => scheme.levelFor(extent, cellSize)
    }
    val kb = bounds.setSpatialBounds(KeyBounds(layout.mapTransform(extent)))
    (zoom, TileLayerMetadata(cellType, layout, extent, crs, kb))
  }

You might be able to restructure something that takes a LayoutLevel explicitly which might look a bit more pretty.

This comment has been minimized.

@mbertrand

mbertrand Jul 7, 2016

Contributor

@lossyrob sorry for the delay, I just updated the branch with your suggested changes.

@pomadchin pomadchin referenced this pull request Jun 20, 2016

Merged

ETL Refactor #1553

10 of 10 tasks complete
@lossyrob

This comment has been minimized.

Member

lossyrob commented Jul 7, 2016

@mbertrand thanks! This is a great change 👍

@lossyrob lossyrob merged commit 12fa1ad into locationtech:master Jul 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lossyrob lossyrob added this to the 0.10.2 milestone Jul 7, 2016

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