Unsigned variable with valid range, min, max bug

cdm/core/src/main/java/ucar/nc2/filter/ConvertMissing.java

@@ -49,7 +49,7 @@ public static ConvertMissing createFromVariable(VariableDS var) {
       validMax = var.convertUnsigned(validRangeAtt.getNumericValue(1), validType).doubleValue();
       hasValidMin = true;
       hasValidMax = true;
-      validRangeDifferentDataType = !validType.equals(var.getDataType());
+      validRangeDifferentDataType = !validType.equals(var.getOriginalDataType());
     }
 
     Attribute validMinAtt = var.findAttribute(CDM.VALID_MIN);
@@ -72,7 +72,10 @@ public static ConvertMissing createFromVariable(VariableDS var) {
       }
     }
 
-    if (validRangeDifferentDataType && !signedness.equals(Signedness.UNSIGNED)) {
+    boolean unsignedBecauseCdmAttr = var.attributes().findAttributeString(CDM.UNSIGNED, "false").equals("true");
+    // skip unscaling if signedness is UNSIGNED and the CDM _Unsigned attribute is true
+    boolean skipUnscale = signedness.equals(Signedness.UNSIGNED) && unsignedBecauseCdmAttr;
+    if (validRangeDifferentDataType && !skipUnscale) {
       // Signal that valid range (or min/max) was specified in unpacked values, so we
       // need to repack those values. Only applies when the DataTypes do not match because
       // the variable is unsigned.

cdm/core/src/test/java/ucar/nc2/dataset/TestScaleOffsetMissingUnsigned.java

@@ -5,6 +5,7 @@
 
 package ucar.nc2.dataset;
 
+import static com.google.common.truth.Truth.assertThat;
 import static java.lang.Float.NaN;
 
 import org.junit.Assert;
@@ -342,4 +343,68 @@ public void testUnsignedOffsetAttribute() throws IOException, URISyntaxException
       Assert.assertEquals(106, var.read().getByte(0)); // -50 + 156 == 106
     }
   }
+
+  @Test
+  public void testScaleOffsetValidRangeDiffTypesOldApi() throws URISyntaxException, IOException {
+    File testResource = new File(getClass().getResource("testScaleOffsetMissingUnsigned.ncml").toURI());
+
+    try (NetcdfDataset ncd = NetcdfDataset.openDataset(testResource.getAbsolutePath(), true, null)) {
+      // Same as scaleOffsetValidMaxMin, but uses valid_range attribute instead of valid_min and valid_max attributes.
+      VariableDS var = (VariableDS) ncd.findVariable("packedUnmatchedType");
+
+      // Packed value of valid min, max should only be used internally to ConvertMissing, so make sure it is
+      // not leaking through
+      assertThat(var.getValidMin()).isNotWithin(0.01).of(127);
+      assertThat(var.getValidMax()).isNotWithin(0.01).of(129);
+      // Make sure unpacked values still make it through
+      assertThat(var.getValidMin()).isWithin(0.01).of(255);
+      assertThat(var.getValidMax()).isWithin(0.01).of(259);
+
+      // This will only work if the unpacked values of valid min/max are used by
+      // ConvertMissing
+      float[] expected = new float[] {NaN, 255, 257, 259};
+      float[] actual = (float[]) var.read().getStorage();
+      for (int i = 0; i < actual.length; i++) {
+        if (var.isInvalidData(actual[i])) {
+          assertThat(actual[i]).isNaN();
+          assertThat(expected[i]).isNaN();
+        } else {
+          assertThat(actual[i]).isNotNaN();
+          assertThat(actual[i]).isWithin(0.01f).of(expected[i]);
+        }
+      }
+    }
+  }
+
+  @Test
+  public void testScaleOffsetValidRangeDiffTypes() throws URISyntaxException, IOException {
+    File testResource = new File(getClass().getResource("testScaleOffsetMissingUnsigned.ncml").toURI());
+
+    try (NetcdfDataset ncd = NetcdfDatasets.openDataset(testResource.getAbsolutePath(), true, null)) {
+      // Same as scaleOffsetValidMaxMin, but uses valid_range attribute instead of valid_min and valid_max attributes.
+      VariableDS var = (VariableDS) ncd.findVariable("packedUnmatchedType");
+
+      // Packed value of valid min, max should only be used internally to ConvertMissing, so make sure it is
+      // not leaking through
+      assertThat(var.getValidMin()).isNotWithin(0.01).of(127);
+      assertThat(var.getValidMax()).isNotWithin(0.01).of(129);
+      // Make sure unpacked values still make it through
+      assertThat(var.getValidMin()).isWithin(0.01).of(255);
+      assertThat(var.getValidMax()).isWithin(0.01).of(259);
+
+      // This will only work if the unpacked values of valid min/max are used by
+      // ConvertMissing
+      float[] expected = new float[] {NaN, 255, 257, 259};
+      float[] actual = (float[]) var.read().getStorage();
+      for (int i = 0; i < actual.length; i++) {
+        if (var.isInvalidData(actual[i])) {
+          assertThat(actual[i]).isNaN();
+          assertThat(expected[i]).isNaN();
+        } else {
+          assertThat(actual[i]).isNotNaN();
+          assertThat(actual[i]).isWithin(0.01f).of(expected[i]);
+        }
+      }
+    }
+  }
 }

cdm/core/src/test/resources/ucar/nc2/dataset/testScaleOffsetMissingUnsigned.ncml

@@ -53,4 +53,12 @@
         <attribute name="add_offset" type="byte" isUnsigned="true" value="-100" />  <!-- 156 -->
         <values>-50</values>
     </variable>
+    <variable name="packedUnmatchedType" type="ubyte" shape="4">
+      <attribute name="scale_factor" type="float" value="2.0" />
+      <attribute name="add_offset" type="float" value="1.0" />
+      <attribute name="valid_range" type="float" value="255 259" />
+      <!-- When opening as a NetcdfDataset, the first value should be missing, where as the remaining
+           three values should be valid. -->
+      <values>126 127 128 129</values>
+    </variable>
 </netcdf>