search

locationtech / geotrellis

GeoTrellis is a geographic data processing engine for high performance applications.
http://geotrellis.io
Other
1.34k stars 361 forks source link

Implicit conversions from ProjectedRaster to Tile create confusing semantics #2829

Open metasim opened 5 years ago

metasim commented 5 years ago

To wit:

scala> import geotrellis.proj4._
scala> import geotrellis.raster._
scala> import geotrellis.spark.tiling._

scala> val foo = ProjectedRaster(ByteConstantTile(1, 1, 1), LatLng.worldExtent, LatLng)
foo: geotrellis.raster.ProjectedRaster[geotrellis.raster.ByteConstantTile] = ProjectedRaster(Raster(ByteConstantTile(1,1,1,int8),Extent(-180.0, -89.99999, 179.99999, 89.99999)),LatLng)

scala> val bar = foo.convert(IntConstantNoDataCellType)
bar: geotrellis.raster.Tile = IntConstantTile(1,1,1,int32)

scala> bar.extent
<console>:41: error: value extent is not a member of geotrellis.raster.Tile
       bar.extent
           ^

I suggest removing these:

https://github.com/locationtech/geotrellis/blob/cb39e0ba1f6d964b522dc3a194ff3fa24fd8efd2/raster/src/main/scala/geotrellis/raster/ProjectedRaster.scala#L38-L44

I also think the ones from tuples should go to, but the argument isn't as strong. Same for Raster[T].

Related: #1780, #2768

pomadchin commented 5 years ago

Would not be better just to have conversion (and other funcs) function properly implemented there?

echeipesh commented 5 years ago

@pomadchin You'd have to define convert on Raster or ProjectedRaster which is of dubious value for weight.

pomadchin commented 5 years ago

Ah indeed; also read a lil bit wrong the issue from phone

metasim commented 5 years ago

I'd actually voice support for having at least mapTile, which Raster has:

def mapTile[A <: CellGrid](f: T => A): ProjectedRaster[A] = ....

At least that way you can get at convert.

I uses convert and interpretAs enough that I wish they were on ProjectedRaster[T], but you'd have to context bound T appropriately, and that does get heavy weight.

metasim commented 5 years ago

Anyone know what happens if you try to @deprecate implicit conversions?

pomadchin commented 5 years ago

@metasim actually a good question: https://github.com/scala/bug/issues/10152 But probably would work as expected.

metasim commented 5 years ago

To followup from above, this is the use case I have a lot of time, especially when GeoTIFFs don't have the NoData value properly set:

val pr = someGeotiff.projectedRaster
val tile = pr.tile.interpretAs(UShortConstantNoDataCellType)
val fixedpr = ProjectedRaster(tile, pr.extent, pr.crs)
pomadchin commented 5 years ago

@metasim 

Welcome to Scala 2.11.12 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_162).
Type in expressions for evaluation. Or try :help.

scala> :paste
// Entering paste mode (ctrl-D to finish)

@deprecated("this method will be removed // dummy conversion", "GeoTrellis")
implicit def intToString(i: Int): String = i.toString

// Exiting paste mode, now interpreting.

intToString: (i: Int)String

scala> val str: String = 25
<console>:12: warning: method intToString is deprecated: this method will be removed // dummy conversion
       val str: String = 25
                         ^
str: String = 25
metasim commented 5 years ago

So how about we start off with just @deprecated, and then in the next breaking release remove them?

pomadchin commented 5 years ago

It looks like 3.0 release is very close :D we can try to get it merged in

metasim commented 5 years ago

Testing it out now; works as advertised:

MultibandTilePolygonalSummaryHandler.scala:41:113: method featureToRaster in object Raster is deprecated: Implicit conversions considered unsafe
...
MatchingRasters.scala:56:33: method projectedToRaster in object ProjectedRaster is deprecated: Implicit conversions considered unsafe

Need to determine a better message tho...