-
Notifications
You must be signed in to change notification settings - Fork 360
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 union to merge methods #3482
base: master
Are you sure you want to change the base?
Add union to merge methods #3482
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
untionF
=> unionFunc
(it took me a while to understand what F stands for here) :D
@@ -154,4 +154,69 @@ trait SinglebandTileMergeMethods extends TileMergeMethods[Tile] { | |||
case _ => | |||
self | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A forgotten comment?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really realted to this PR, but it looks like it's the time to import cats.syntax.semigroup._
:D
Overview
This PR adds a tile merging function,
union
which 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:
![image](https://user-images.githubusercontent.com/1977405/182711914-096bfc76-af00-4959-8343-642f17b77da4.png)
Union of overlapping tiles with a function applied to average overlapping cell values:
![image](https://user-images.githubusercontent.com/1977405/182712053-005ea715-8c39-4c02-ae85-d043816f44c6.png)
Closes #3365