combine when implementing a DelegatingTile incorrectly handles NoData

[metasim]

// Regular tile
val t1 = ArrayTile(Array.fill(4)(1), 2, 2)
// Tile with single NoData value
val t2 = {
  val d = Array.fill(4)(2)
  d(1) = geotrellis.raster.NODATA
  ArrayTile(d, 2, 2)
}

val d1 = new DelegatingTile {
  override def delegate: Tile = t1
}
val d2 = new DelegatingTile {
  override def delegate: Tile = t2
}
/** Counts the number of non-NoData cells in a tile */
case object CountData {
  def apply(t: Tile) = {
    var count: Long = 0
    t.dualForeach(
      z ⇒ if(isData(z)) count = count + 1
    ) (
      z ⇒ if(isData(z)) count = count + 1
    )
    count
  }
}

// Confirm counts
CountData(t1) should be (4L)
CountData(t2) should be (3L)
CountData(d1) should be (4L)
CountData(d2) should be (3L)

// Standard Add evaluates `x + NoData` as `NoData`
CountData(Add(t1, t2)) should be (3L)
CountData(Add(d1, d2)) should be (3L)
// Is commutative
CountData(Add(t2, t1)) should be (3L)
CountData(Add(d2, d1)) should be (3L)

/** BiasedAdd is evaluates `x + NoData` as `x` */
case object BiasedAdd extends LocalTileBinaryOp {
  def op(z1: Int, z2: Int): Int = z1 + z2
  def op(z1: Double, z2: Double): Double = z1+z2

  def combine(z1: Int, z2: Int): Int =
    if (isNoData(z1) && isNoData(z2)) raster.NODATA
    else if (isNoData(z1))
      z2
    else if (isNoData(z2))
      z1
    else
      op(z1, z2)

  def combine(z1: Double, z2: Double): Double =
    if (isNoData(z1) && isNoData(z2)) raster.doubleNODATA
    else if (isNoData(z1))
      z2
    else if (isNoData(z2))
      z1
    else
      op(z1, z2)
}

// With BiasedAdd, all cells should be data cells
CountData(BiasedAdd(t1, t2)) should be (4L) // <-- passes
CountData(BiasedAdd(d1, d2)) should be (4L) // <-- fails
// Should be commutative.
CountData(BiasedAdd(t2, t1)) should be (4L) // <-- passes
CountData(BiasedAdd(d2, d1)) should be (4L) // <-- fails

I think this is a reemergence of #2907, but when DelegatingTile is used. Basically, the fallback/default implementation of ArrayTile.combine, when given something that's not a recognized type, ends up providing an implementation that makes assumptions about NoData handling that should be deferred to the combine operator.

Also suspicious that these two implementations are so different:

geotrellis/raster/src/main/scala/geotrellis/raster/ArrayTile.scala

Lines 232 to 238 in c99dc66

	this.map((col, row, z) => {
	if (isNoData(z)) z
	else {
	val v = t.get(col, row)
	if (isNoData(v)) v
	else f(z, v)
	}

geotrellis/raster/src/main/scala/geotrellis/raster/ArrayTile.scala

Lines 283 to 284 in c99dc66

	case t =>
	this.mapDouble((col, row, z) => f(z, t.getDouble(col, row)))

Looks like I'm the culprit of this code being broken, so planning on doing a PR. One solution is to add yet another switch based on type, but feels like a dead end. On the other hand, I'm loath to change something as fundamental as the combine methods without knowing there's extensive test coverage over the data space. @echeipesh @pomadchin @lossyrob: Any advice/warnings there is appreciated.

[metasim]

Note: if I change the input to be:

// Regular tile
val t1 = ArrayTile(Array.fill(4)(1.0), 2, 2)
// Tile with single NoData value
val t2 = {
  val d = Array.fill(4)(2.0)
  d(1) = geotrellis.raster.doubleNODATA
  ArrayTile(d, 2, 2)
}

(make it a floating point tile), the test passes, so the fix may be as simple as making the integral form take the same approach as the floating point form....