Skip to content
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

fix scale offset applied twice #1247

Merged
merged 10 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,7 @@ protected void readValues() {
}

coords = (double[]) data.get1DJavaArray(DataType.DOUBLE);
this.wasRead = true;
// IndexIterator iter = data.getIndexIterator();
// while (iter.hasNext())
// coords[count++] = iter.getDoubleNext();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,17 @@ private CoordinateAxis1DTime(NetcdfDataset ncd, VariableDS org, Formatter errMes
int ncoords = (int) org.getSize();
List<CalendarDate> result = new ArrayList<>(ncoords);

Array data = org.read();
if (org instanceof CoordinateAxis1D) {
coords = ((CoordinateAxis1D) org).getCoordValues();
} else {
Array data = org.read();
coords = (double[]) data.get1DJavaArray(DataType.DOUBLE);
}
this.wasRead = true;

int count = 0;
IndexIterator ii = data.getIndexIterator();
for (int i = 0; i < ncoords; i++) {
double val = ii.getDoubleNext();
double val = coords[i];
if (Double.isNaN(val))
continue; // WTF ??
result.add(helper.makeCalendarDateFromOffset(val));
Expand All @@ -355,12 +360,11 @@ private CoordinateAxis1DTime(NetcdfDataset ncd, VariableDS org, Formatter errMes
setDimension(0, localDim);

// set the shortened values
Array shortData = Array.factory(data.getDataType(), new int[] {count});
Array shortData = Array.factory(org.getDataType(), new int[] {count});
Index ima = shortData.getIndex();
int count2 = 0;
ii = data.getIndexIterator();
for (int i = 0; i < ncoords; i++) {
double val = ii.getDoubleNext();
double val = coords[i];
if (Double.isNaN(val))
continue;
shortData.setDouble(ima.set0(count2), val);
Expand Down
8 changes: 6 additions & 2 deletions cdm/core/src/main/java/ucar/nc2/dataset/NetcdfDataset.java
Original file line number Diff line number Diff line change
Expand Up @@ -1344,12 +1344,16 @@ public void addCoordinateTransform(CoordinateTransform ct) {
public CoordinateAxis addCoordinateAxis(VariableDS v) {
if (v == null)
return null;

final List<CoordinateAxis> coordCopy = new ArrayList<>(coordAxes);
tdrwenski marked this conversation as resolved.
Show resolved Hide resolved

CoordinateAxis oldVar = findCoordinateAxis(v.getFullName());
if (oldVar != null)
coordAxes.remove(oldVar);
coordCopy.remove(oldVar);

CoordinateAxis ca = (v instanceof CoordinateAxis) ? (CoordinateAxis) v : CoordinateAxis.factory(this, v);
coordAxes.add(ca);
coordCopy.add(ca);
this.coordAxes = coordCopy;

if (v.isMemberOfStructure()) {
Structure parentOrg = v.getParentStructure(); // gotta be careful to get the wrapping parent
Expand Down
27 changes: 19 additions & 8 deletions cdm/core/src/main/java/ucar/nc2/dataset/VariableDS.java
Original file line number Diff line number Diff line change
Expand Up @@ -436,12 +436,12 @@ public void setCaching(boolean caching) {
@Override
protected Array _read() throws IOException {
Array result;

// check if already cached - caching in VariableDS only done explicitly by app
if (hasCachedData())
if (hasCachedData()) {
result = super._read();
else
} else {
result = proxyReader.reallyRead(this, null);
}

return convert(result);
}
Expand All @@ -450,15 +450,16 @@ protected Array _read() throws IOException {
@Override
protected Array _read(Section section) throws IOException, InvalidRangeException {
// really a full read
if ((null == section) || section.computeSize() == getSize())
if ((null == section) || section.computeSize() == getSize()) {
return _read();
}

Array result;
if (hasCachedData())
if (hasCachedData()) {
result = super._read(section);
else
} else {
result = proxyReader.reallyRead(this, section, null);

}
return convert(result);
}

Expand Down Expand Up @@ -840,6 +841,7 @@ protected VariableDS(Builder<?> builder, Group parentGroup) {

this.orgFileTypeId = builder.orgFileTypeId;
this.enhanceProxy = new EnhancementsImpl(this, builder.units, builder.getDescription());
this.scaleOffset = builder.scaleOffset;

createEnhancements();

Expand All @@ -858,7 +860,9 @@ private void createEnhancements() {
this.dataType = unsignedConversion != null ? unsignedConversion.getOutType() : dataType;
}
if (this.enhanceMode.contains(Enhance.ApplyScaleOffset) && (dataType.isNumeric() || dataType == DataType.CHAR)) {
this.scaleOffset = ScaleOffset.createFromVariable(this);
if (this.scaleOffset == null) {
this.scaleOffset = ScaleOffset.createFromVariable(this);
}
this.dataType = scaleOffset != null ? scaleOffset.getScaledOffsetType() : this.dataType;
}
Attribute standardizerAtt = findAttribute(CDM.STANDARDIZE);
Expand Down Expand Up @@ -902,6 +906,10 @@ protected Builder<?> addLocalFieldsToBuilder(Builder<? extends Builder<?>> build
builder.setOriginalVariable(this.orgVar).setOriginalDataType(this.orgDataType).setOriginalName(this.orgName)
.setOriginalFileTypeId(this.orgFileTypeId).setEnhanceMode(this.enhanceMode).setUnits(this.enhanceProxy.units)
.setDesc(this.enhanceProxy.desc);
builder.scaleOffset = this.scaleOffset;
if (this.coordSysNames != null) {
this.coordSysNames.stream().forEach(s -> builder.addCoordinateSystemName(s));
}

return (VariableDS.Builder<?>) super.addLocalFieldsToBuilder(builder);
}
Expand Down Expand Up @@ -944,6 +952,8 @@ public static abstract class Builder<T extends Builder<T>> extends Variable.Buil
private boolean fillValueIsMissing = NetcdfDataset.fillValueIsMissing;
private boolean missingDataIsMissing = NetcdfDataset.missingDataIsMissing;

private ScaleOffset scaleOffset;

private boolean built;

protected abstract T self();
Expand Down Expand Up @@ -1040,6 +1050,7 @@ public T copyFrom(VariableDS.Builder<?> builder) {
setFillValueIsMissing(builder.fillValueIsMissing);
setInvalidDataIsMissing(builder.invalidDataIsMissing);
setMissingDataIsMissing(builder.missingDataIsMissing);
this.scaleOffset = builder.scaleOffset;
this.orgVar = builder.orgVar;
this.orgDataType = builder.orgDataType;
this.orgFileTypeId = builder.orgFileTypeId;
Expand Down
3 changes: 3 additions & 0 deletions cdm/core/src/main/java/ucar/nc2/filter/ScaleOffset.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,16 @@ public static ScaleOffset createFromVariable(VariableDS var) {
if (scaleAtt != null && !scaleAtt.isString()) {
scaleType = FilterHelpers.getAttributeDataType(scaleAtt, signedness);
scale = 1 / (var.convertUnsigned(scaleAtt.getNumericValue(), scaleType).doubleValue());
var.remove(scaleAtt);
}

Attribute offsetAtt = var.findAttribute(CDM.ADD_OFFSET);
if (offsetAtt != null && !offsetAtt.isString()) {
offsetType = FilterHelpers.getAttributeDataType(offsetAtt, signedness);
offset = var.convertUnsigned(offsetAtt.getNumericValue(), offsetType).doubleValue();
var.remove(offsetAtt);
}

if (scale != DEFAULT_SCALE || offset != DEFAULT_OFFSET) {
DataType scaledOffsetType =
FilterHelpers.largestOf(var.getUnsignedConversionType(), scaleType, offsetType).withSignedness(signedness);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,8 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import ucar.nc2.Attribute;
import ucar.nc2.Dimension;
import ucar.nc2.NetcdfFile;
import ucar.nc2.Variable;
import ucar.nc2.VariableSimpleIF;

import ucar.nc2.*;
import ucar.nc2.constants.CDM;
import ucar.nc2.constants.FeatureType;
import ucar.nc2.dataset.CoordinateAxis;
Expand Down Expand Up @@ -113,22 +110,25 @@ public DtCoverageDataset(NetcdfDataset ncd) throws IOException {
* @throws java.io.IOException on read error
*/
public DtCoverageDataset(NetcdfDataset ncd, Formatter parseInfo) throws IOException {
this.ncd = ncd;

Set<NetcdfDataset.Enhance> enhance = ncd.getEnhanceMode();
if (enhance == null || enhance.isEmpty())
if (enhance == null || enhance.isEmpty()) {
enhance = NetcdfDataset.getDefaultEnhanceMode();
ncd = NetcdfDatasets.enhance(ncd, enhance, null);
}
this.ncd = NetcdfDatasets.enhance(ncd, enhance, null);

DtCoverageCSBuilder facc = DtCoverageCSBuilder.classify(ncd, parseInfo);
if (facc != null)
this.coverageType = facc.type;
// sort by largest size first
List<CoordinateSystem> csList = new ArrayList<>(ncd.getCoordinateSystems());
csList.sort((o1, o2) -> o2.getCoordinateAxes().size() - o1.getCoordinateAxes().size());

Map<String, Gridset> csHash = new HashMap<>();
for (CoordinateSystem cs : ncd.getCoordinateSystems()) {
for (CoordinateSystem cs : csList) {
DtCoverageCSBuilder fac = new DtCoverageCSBuilder(ncd, cs, parseInfo);
if (fac.type == null)
if (fac.type == null) {
continue;
}
if (this.coverageType == null) {
this.coverageType = fac.type;
}
DtCoverageCS ccs = fac.makeCoordSys();
if (ccs == null)
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public List<CoordinateTransform> getCoordTransforms() {
private CoordinatesHelper(Builder builder, NetcdfDataset ncd) {
List<CoordinateAxis> axes = new ArrayList<>();
addAxes(ncd.getRootGroup(), axes);
this.coordAxes = ImmutableList.copyOf(axes);
coordAxes = ImmutableList.copyOf(axes);

coordTransforms =
builder.coordTransforms.stream().map(ct -> ct.build(ncd)).filter(Objects::nonNull).collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ public void testWrite() throws Exception {
}

Array readEnhanced;

// read the packed form, enhance using scale/offset, compare to original
try (NetcdfDataset ncd = NetcdfDatasets.openDataset(filename)) {
VariableDS vs = (VariableDS) ncd.findVariable("packed");
Expand Down Expand Up @@ -133,15 +132,14 @@ private void doSubset(String filename) throws IOException, InvalidRangeException
}
}


// Asserts that "scale_factor" is applied to "_FillValue".
// This test demonstrated the bug in https://github.com/Unidata/thredds/issues/1065.
@Test
public void testScaledFillValue() throws URISyntaxException, IOException {
File testResource = new File(getClass().getResource("testScaledFillValue.ncml").toURI());

// LOOK removeEnhancement does not work in new
try (NetcdfDataset ncd = NetcdfDataset.openDataset(testResource.getAbsolutePath(), true, null)) {
try (NetcdfDataset ncd = NetcdfDatasets.openDataset(testResource.getAbsolutePath(), true, null)) {
VariableDS fooVar = (VariableDS) ncd.findVariable("foo");

double expectedFillValue = .99999;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public void testStandaloneEnhance() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isFalse();
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand All @@ -91,8 +90,7 @@ public void testStandaloneEnhanceDataset() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isFalse();
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand All @@ -110,8 +108,6 @@ public void testStandaloneDoubleEnhanceDataset() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand Down
9 changes: 3 additions & 6 deletions cdm/core/src/test/java/ucar/nc2/ncml/TestEnhance.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ public void testStandaloneEnhance() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isFalse();
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand All @@ -87,8 +86,7 @@ public void testStandaloneEnhanceDataset() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isFalse();
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand All @@ -106,8 +104,7 @@ public void testStandaloneDoubleEnhanceDataset() throws IOException {
Variable scaledvar = ncfile.findVariable("scaledvar");
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
assertThat(scaledvar.attributes().findAttributeDouble("scale_factor", 1.0)).isEqualTo(2.0);
assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isFalse();
assertThat(scaledvar.readScalarFloat()).isEqualTo(12.0f);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,8 @@ public void testUnpackDataTutorial() throws IOException {
assertThat((Object) scaledvar).isNotNull();
assertThat(scaledvar.getDataType()).isEqualTo(DataType.FLOAT);

assertThat(scaledvar.attributes().hasAttribute("scale_factor")).isTrue();
double scale_factor = scaledvar.attributes().findAttributeDouble("scale_factor", 1.0);
assertThat(scaledvar.attributes().hasAttribute("add_offset")).isTrue();
double add_offset = scaledvar.attributes().findAttributeDouble("add_offset", 1.0);

double unpacked_data =
NetcdfDatasetTutorial.unpackData(scaledvar.readScalarShort(), scale_factor, add_offset);
NetcdfDatasetTutorial.unpackData(scaledvar.readScalarShort(), 1.0, 0.0);
assertThat(unpacked_data).isNotNaN();
}
}
Expand Down
Loading