MapKeyTransform method aliases #2334

fosskers · 2017-08-17T17:17:41Z

TODO

Named aliases for each .apply variant
Provide these transformations directly in the types they affect (where possible)
Deprecations?

Motivation

MapKeyTransform is a "worker class", it does not itself represent data. It provides mechanisms for transformations involving SpatialKeys, namely:

discovering the key corresponding to some Geometry
discovering the Extent that corresponds to some key
discovering the set of keys (GridBounds) that corresponds to some Geometry envelope (an Extent)

Until now, each of these transformations occurred through a very overloaded .apply method, making MapKeyTransform somewhat of a mysterious black box. This PR adds sensibly named aliases for each of these transformations, like keyToExtent: SpatialKey => Extent.

Further Work

Duplicate Deprecations

If it's true that reducing API surface area increases discoverability and improves usability, is it necessary to have:

both apply: Point => SpatialKey and apply: (Double, Double) => SpatialKey
all of apply: (Int, Int) => Extent and apply: SpatialKey => Extent and apply[K: SpatialComponent]: K => Extent

given that the extra forms are simple pure transformations that could occur in application code?
For instance, as currently exists in the code base:

 def apply[K: SpatialComponent](key: K): Extent = apply(key.getComponent[SpatialKey])

My recommendation is to keep SpatialKey => Extent and Point => SpatialKey and deprecate the duplicates.

`.apply` Deprecations

Echoing the "surface area" argument, would it be prudent to deprecate the .apply methods as well, given that they were the original source of confusion? Replacing their usage with the named aliases within GT couldn't be more than a 15 minute find-replace job.

`geotrellis-core`

Each of these transformations can be modelled by the form A.someAToB: LayoutDefinition => B, where the data of the A is used implicitly inside the method. Here, the transformation is a method on A, not a function through MapKeyTransform. For the case of SpatialKey and Extent, it's: SpatialKey.toExtent: LayoutDefinition => Extent. This PR provides this method as well, so that users don't need to go through a MapKeyTransform.

Unfortunately, due to the way the project is structured, we can't provide type-local methods for the other types without incurring circular dependencies:

Point.toKey: LayoutDefinition => SpatialKey
- LayoutDefinition lives in geotrellis.spark.tiling
- SpatialKey lives in geotrellis.spark
- Point lives in geotrellis.vector
GridBounds.toExtent: LayoutDefinition => Extent
- GridBounds lives in geotrellis.raster
- Extent lives in geotrellis.vector

and so on. So MapKeyTransform who lives in geotrellis.spark.tiling has to act as a sort of
"type broker", a place for all these necessary transformations to live. This (and other issues) would be solved by a central geotrellis-core package, which would contain types useful to any conceivable GeoTrellis application (i.e. one shouldn't need to depend on spark just to get access to SpatialKey). This way, we could get rid of MapKeyTransform, and each transformation could occur directly on the class in question.

- I had hoped to add more methods like this, so that users would never need to use MapKeyTransform directly. Unfortunately, due to `LayoutDefinition` and `SpatialKey` living in the `geotrellis-spark` package, attempting that would result in some weird circular dependencies. We would benefit from a central `geotrellis-types` or `geotrellis-core` package.

echeipesh · 2017-08-23T16:10:59Z

spark/src/main/scala/geotrellis/spark/tiling/MapKeyTransform.scala

+  def apply(p: Point): SpatialKey = apply(p.x, p.y)
+
+  /** Fetch the [[SpatialKey]] that corresponds to some coordinates in some CRS on the Earth. */
+  def pointCoordsToKey(x: Double, y: Double): SpatialKey = apply(x, y)


This feels like a place where overloading is helpful to overload between pointToKey(Point(3,5)) and pointToKey(x=3,y=5) are both unsurprising and more discoverable.

This I've added.

echeipesh · 2017-08-23T16:12:25Z

spark/src/main/scala/geotrellis/spark/tiling/MapKeyTransform.scala

-  def apply[K: SpatialComponent](key: K): Extent = {
-    apply(key.getComponent[SpatialKey])
-  }
+  def keyLikeToExtent[K: SpatialComponent](key: K): Extent = apply(key)


Hmm, this could actually take care of the SpatialKey case just fine. Do you think its more clear to have an explicit function not based on SpatialComponent?

Yeah, I think SpatialKey => Extent signals its intent better. If the user knows what the purpose of SpatialComponent is, then should be able to make the getComponent call themselves (if say their key type was SpaceTimeKey).

I think a buy that, what are some of the impacted lines in our codebase for reference ?

echeipesh · 2017-10-05T15:50:07Z

spark/src/main/scala/geotrellis/spark/tiling/MapKeyTransform.scala

-  def apply(key: SpatialKey): Extent =
-    apply(key.col, key.row)
+  /** 'col' and 'row' correspond to a [[SpatialKey]] column and row in some grid. */
+  def coordsToExtent(col: Int, row: Int): Extent = apply(col, row)


Can this be keyToExtent overload? coords is a little overloaded meaning, is it pixel cords, tile cords, map cords, a bit of ambiguity.

I don't think those are the same thing. See the apply that it calls out to right below it.

lossyrob · 2017-10-05T18:08:44Z

spark/src/main/scala/geotrellis/spark/SpatialKey.scala

 import org.apache.spark.rdd.RDD

 /** A SpatialKey designates the spatial positioning of a layer's tile. */
 case class SpatialKey(col: Int, row: Int) extends Product2[Int, Int] {
  def _1 = col
  def _2 = row
+
+  /** Retrieve the [[Extent]] that corresponds to this key, given a layout. */
+  def tileExtent(layout: LayoutDefinition): Extent = layout.mapTransform(this)


The reference to tile here makes this key, which is agnostic of Value type, seem like it's specific to Tile. We've been avoiding placing specific language on things that could be generally useful, e.g. to point clouds that are stored based on spatial keys.

Hmm, thats tough. SpatialKey covers a rectangular, tile-like area on the map based on LayoutDefinition.

lossyrob · 2017-10-05T18:10:36Z

spark/src/main/scala/geotrellis/spark/tiling/MapKeyTransform.scala

+  def apply[K: SpatialComponent](key: K): Extent = apply(key.getComponent[SpatialKey])
+
+  /** Get the [[Extent]] corresponding to a [[SpatialKey]] in some zoom level. */
+  def tileExtent(key: SpatialKey): Extent = apply(key)


As mentioned above, I think we should keep away from naming things with tile.

Suggestion: keyExtent

It was previously toExtent. Which do you think is better?

After a talk with @echeipesh , the current vote is SpatialKey.extent (maximum minimalism, the word "key" doesn't need to be repeated since that's implied by the parent class), and keyToExtent for the MapKeyTransform methods so that they match the naming of their sibling methods. Thoughts?

- And the underlying methods in `MapKeyTransform` have been renamed `keyToExtent` to match the naming scheme of the other methods there.

fosskers added 2 commits August 17, 2017 09:33

MapKeyTransform - Add meaningfully named aliases for .apply

c212386

fosskers mentioned this pull request Aug 21, 2017

Toward GeoTrellis 2.0 #2341

Closed

echeipesh reviewed Aug 23, 2017

View reviewed changes

echeipesh added this to the 1.2 milestone Oct 2, 2017

fosskers added 2 commits October 5, 2017 07:26

Deprecate MapKeyTransform.apply[K: SpatialComponent]: K => Extent

93fb294

Account for deprecation in the previous commit

226a209

echeipesh reviewed Oct 5, 2017

View reviewed changes

keyToExtent -> tileExtent

3ed3317

lossyrob suggested changes Oct 5, 2017

View reviewed changes

SpatialKey.tileExtent -> SpatialKey.extent

4bd85ec

- And the underlying methods in `MapKeyTransform` have been renamed `keyToExtent` to match the naming scheme of the other methods there.

lossyrob approved these changes Oct 6, 2017

View reviewed changes

echeipesh merged commit f5221e4 into master Oct 6, 2017

fosskers deleted the renamed-mt-methods branch November 8, 2017 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MapKeyTransform method aliases #2334

MapKeyTransform method aliases #2334

fosskers commented Aug 17, 2017 •

edited

Loading

echeipesh Aug 23, 2017

fosskers Oct 5, 2017

echeipesh Aug 23, 2017

fosskers Aug 23, 2017

echeipesh Oct 5, 2017

echeipesh Oct 5, 2017 •

edited

Loading

fosskers Oct 5, 2017

lossyrob Oct 5, 2017

echeipesh Oct 5, 2017

lossyrob Oct 5, 2017

fosskers Oct 5, 2017

fosskers Oct 5, 2017

lossyrob Oct 5, 2017

MapKeyTransform method aliases #2334

MapKeyTransform method aliases #2334

Conversation

fosskers commented Aug 17, 2017 • edited Loading

TODO

Motivation

Further Work

Duplicate Deprecations

.apply Deprecations

geotrellis-core

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

echeipesh Oct 5, 2017 • 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

Choose a reason for hiding this comment

fosskers commented Aug 17, 2017 •

edited

Loading

`.apply` Deprecations

`geotrellis-core`

echeipesh Oct 5, 2017 •

edited

Loading