Repository navigation
Add union to merge methods - #3482
Conversation
pomadchin
left a comment
pomadchin
left a comment
There was a problem hiding this comment.
LGTM! left a couple of questions to clarify and we're pretty much g2g
| * @param method The resampling method | ||
| * @return A new Raster, the result of the merge | ||
| */ | ||
| def union(other: Raster[T], method: ResampleMethod, unionF: (Option[Double], Option[Double]) => Double): Raster[T] = |
There was a problem hiding this comment.
untionF => unionFunc (it took me a while to understand what F stands for here) :D
| } | ||
|
|
||
| def union(extent: Extent, otherExtent: Extent, other: Tile, method: ResampleMethod, unionF: (Option[Double], Option[Double]) => Double): Tile = { | ||
| val unionInt = (l: Option[Double], r: Option[Double]) => unionF(l, r).toInt |
There was a problem hiding this comment.
Hm, mb the signature (l: Option[Int], r: Option[Int]) => Int is a bit more clear, any way expensive casts occur. I'm actually wondering what's the cost of those.
| cfor(0)(_ < targetRE.cols, _ + 1) { col => | ||
| val (x,y) = targetRE.gridToMap(col, row) | ||
| val (l,r) = (interpolateLeft(x, y), interpolateRight(x, y)) | ||
| mutableTile.set(col, row, unionInt(Some(l.toDouble), Some(r.toDouble))) |
There was a problem hiding this comment.
Set only if isData?
val v = unionInt(l.some, r.some)
if(isData(v)) mutableTile.set(col, row, v)It's actually a debatable question, but the NoData handling for Doubles and Ints is different and can be hard to capture for users with the (Double, Double) => Double merge function passed. Macro relies on a type, users should figure out themselfes the underlying nodata handling logic.
| cfor(0)(_ < targetRE.cols, _ + 1) { col => | ||
| val (x,y) = targetRE.gridToMap(col, row) | ||
| val (l,r) = (interpolateLeft(x, y), interpolateRight(x, y)) | ||
| mutableTile.setDouble(col, row, unionF(Some(l), Some(r))) |
There was a problem hiding this comment.
isData handling as well? (in case it's not implied that it's incapuslated into the union function definition)
| mutableTile.setDouble(col, row, unionF(maybeL, maybeR)) | ||
| } | ||
| } | ||
| case _ => | ||
| val interpolateLeft: (Double, Double) => Int = Resample(method, self, extent, targetCS).resample _ | ||
| val interpolateRight: (Double, Double) => Int = Resample(method, other, otherExtent, targetCS).resample _ | ||
| cfor(0)(_ < targetRE.rows, _ + 1) { row => | ||
| cfor(0)(_ < targetRE.cols, _ + 1) { col => | ||
| val (x,y) = targetRE.gridToMap(col, row) | ||
| val l = interpolateLeft(x, y) | ||
| val r = interpolateRight(x, y) | ||
| val maybeL = if (isNoData(l)) None else Some(l.toDouble) | ||
| val maybeR = if (isNoData(r)) None else Some(r.toDouble) | ||
| mutableTile.set(col, row, unionInt(maybeL, maybeR)) |
There was a problem hiding this comment.
Should not there be extra isNoData(self.getDouble(col, row)) checks? (in case it's not implied that it's incapuslated into the union function definition)
| case BitCellType | ByteCellType | UByteCellType | ShortCellType | UShortCellType | IntCellType => | ||
| val interpolateLeft: (Double, Double) => Int = Resample(method, self, extent, targetCS).resample _ | ||
| val interpolateRight: (Double, Double) => Int = Resample(method, other, otherExtent, targetCS).resample _ | ||
| // Assume 0 as the transparent value |
There was a problem hiding this comment.
Is it really an assumption here? In the original merge logic we checked it to merge new pixels into the new tile in case it's no data.
| val maybeL = if (isNoData(l)) None else Some(l.toDouble) | ||
| val maybeR = if (isNoData(r)) None else Some(r.toDouble) | ||
| mutableTile.set(col, row, unionInt(maybeL, maybeR)) | ||
| //if (l!=r) println(s"x => ${x}, y => ${y}, col => ${col}, row => ${row} | l,r => ${l}, ${r}") |
| method: ResampleMethod, | ||
| unionF: (Option[Double], Option[Double]) => Double | ||
| ): TileFeature[T, D] = | ||
| TileFeature(self.tile.union(extent, otherExtent, other.tile, method, unionF), Semigroup[D].combine(self.data, other.data)) |
There was a problem hiding this comment.
Not really realted to this PR, but it looks like it's the time to import cats.syntax.semigroup._ :D
2 participants
Overview
This PR adds a tile merging function,
unionwhich combines two input tiles so as to preserve all non-overlapping regions.Checklist
Demo
Union of two tiles; tile 1 with extent 0,0,1,1 and tile2 with extent 2,2,2.2,2.2. Output tile with extent of 0,0,2.2,2.2:
Union of overlapping tiles with a function applied to average overlapping cell values:
Closes #3365