PR feedback on NoData check for masking

Signed-off-by: Jason T. Brown <[email protected]>

Author: vpipkt

core/src/main/scala/org/locationtech/rasterframes/expressions/transformers/Mask.scala

@@ -23,7 +23,7 @@ package org.locationtech.rasterframes.expressions.transformers
 
 import com.typesafe.scalalogging.Logger
 import geotrellis.raster
-import geotrellis.raster.{CellType, NoNoData, Tile}
+import geotrellis.raster.{NoNoData, Tile}
 import geotrellis.raster.mapalgebra.local.{Undefined, InverseMask ⇒ gtInverseMask, Mask ⇒ gtMask}
 import org.apache.spark.sql.catalyst.analysis.TypeCheckResult
 import org.apache.spark.sql.catalyst.analysis.TypeCheckResult.{TypeCheckFailure, TypeCheckSuccess}
@@ -74,13 +74,8 @@ abstract class Mask(val left: Expression, val middle: Expression, val right: Exp
     implicit val tileSer = TileUDT.tileSerializer
     val (targetTile, targetCtx) = tileExtractor(targetExp.dataType)(row(targetInput))
 
-    // Which of these is preferable? companion object Seq contains or pattern match on trait?
-//    assert(!CellType.noNoDataCellTypes.contains(targetTile.cellType), maskErrorStr)
-    targetTile.cellType match {
-      case _: NoNoData ⇒ assert(false,
-        s"Input data expression ${left.prettyName} must have a CellType with NoData defined in order to perform a masking operation. Found CellType ${targetTile.cellType.toString()}.")
-      case _ ⇒
-    }
+    require(! targetTile.cellType.isInstanceOf[NoNoData],
+      s"Input data expression ${left.prettyName} must have a CellType with NoData defined in order to perform a masking operation. Found CellType ${targetTile.cellType.toString()}.")
 
     val (maskTile, maskCtx) = tileExtractor(maskExp.dataType)(row(maskInput))
 

…/rasterframes/MaskingFunctionsSpec.scala …mes/functions/MaskingFunctionsSpec.scalacore/src/test/scala/org/locationtech/rasterframes/MaskingFunctionsSpec.scala renamed to core/src/test/scala/org/locationtech/rasterframes/functions/MaskingFunctionsSpec.scala core/src/test/scala/org/locationtech/rasterframes/MaskingFunctionsSpec.scala renamed to core/src/test/scala/org/locationtech/rasterframes/functions/MaskingFunctionsSpec.scala

@@ -19,24 +19,20 @@
  *
  */
 
-package org.locationtech.rasterframes
+package org.locationtech.rasterframes.functions
 
 import geotrellis.raster
 import geotrellis.raster._
 import geotrellis.raster.testkit.RasterMatchers
 import org.apache.spark.sql.Encoders
 import org.apache.spark.sql.functions._
-import org.locationtech.rasterframes.expressions.accessors.ExtractTile
-import org.locationtech.rasterframes.stats._
 import org.locationtech.rasterframes.tiles.ProjectedRasterTile
-
-
-import geotrellis.raster.testkit.RasterMatchers
+import org.locationtech.rasterframes._
 
 class MaskingFunctionsSpec extends TestEnvironment with RasterMatchers {
+  import ProjectedRasterTile.prtEncoder
   import TestData._
   import spark.implicits._
-  import ProjectedRasterTile.prtEncoder
 
   implicit val pairEnc = Encoders.tuple(ProjectedRasterTile.prtEncoder, ProjectedRasterTile.prtEncoder)
   implicit val tripEnc = Encoders.tuple(ProjectedRasterTile.prtEncoder, ProjectedRasterTile.prtEncoder, ProjectedRasterTile.prtEncoder)
@@ -112,10 +108,12 @@ class MaskingFunctionsSpec extends TestEnvironment with RasterMatchers {
       val df = Seq(TestData.projectedRasterTile(5, 5, 42, TestData.extent, TestData.crs,
         noNoDataCellType))
         .toDF("tile")
-      // if this had NoData defined would be a no-op because the tile is all datacells
-      lazy val result = df.select(rf_mask($"tile", $"tile")).collect()
-      an[AssertionError] should be thrownBy(result)
+
+      an [IllegalArgumentException] should be thrownBy {
+        df.select(rf_mask($"tile", $"tile")).collect()
+      }
     }
+
   }
 
   describe("mask by value") {
@@ -284,7 +282,7 @@ class MaskingFunctionsSpec extends TestEnvironment with RasterMatchers {
 
       def checker(colName: String, valFilter: Int, assertValue: Int): Unit = {
         // print this so we can see what's happening if something  wrong
-        println(s"${colName} should be ${assertValue} for qa val ${valFilter}")
+        logger.debug(s"${colName} should be ${assertValue} for qa val ${valFilter}")
         result.filter($"val" === lit(valFilter))
           .select(col(colName))
           .as[ProjectedRasterTile]
@@ -377,7 +375,7 @@ class MaskingFunctionsSpec extends TestEnvironment with RasterMatchers {
         val printOutcome = if (resultIsNoData) "all NoData cells"
         else "all data cells"
 
-        println(s"${columnName} should contain ${printOutcome} for qa val ${maskValueFilter}")
+        logger.debug(s"${columnName} should contain ${printOutcome} for qa val ${maskValueFilter}")
         val resultDf = result
           .filter($"val" === lit(maskValueFilter))
 
@@ -386,9 +384,7 @@ class MaskingFunctionsSpec extends TestEnvironment with RasterMatchers {
           .first()
 
         val dataTile = resultDf.select(col(columnName)).as[ProjectedRasterTile].first()
-        println(s"\tData tile values for col ${columnName}: ${dataTile.toArray().mkString(",")}")
-        //        val celltype = resultDf.select(rf_cell_type(col(columnName))).as[CellType].first()
-        //        println(s"Cell type for col ${columnName}: ${celltype}")
+        logger.debug(s"\tData tile values for col ${columnName}: ${dataTile.toArray().mkString(",")}")
 
         resultToCheck should be (resultIsNoData)
       }

docs/src/main/paradox/release-notes.md

@@ -8,7 +8,7 @@
 * _Breaking_: `rf_spatial_index` renamed `rf_xz2_index` to differentiate between XZ2 and Z2 variants.
 * Added `withSpatialIndex` to RasterSourceDataSource to pre-partition tiles based on tile extents mapped to a Z2 space-filling curve 
 * Add `rf_mask_by_bit`, `rf_mask_by_bits` and `rf_local_extract_bits` to deal with bit packed quality masks. Updated the masking documentation to demonstrate the use of these functions.
-* Throw an `AssertionError` when attempting to apply a mask to a `Tile` whose `CellType` has no NoData defined. ([#409](https://github.com/locationtech/rasterframes/issues/384))
+* Throw an `IllegalArgumentException` when attempting to apply a mask to a `Tile` whose `CellType` has no NoData defined. ([#409](https://github.com/locationtech/rasterframes/issues/384))
 
 ### 0.8.4