Skip to content

Fix for clipping and rounding issues #267

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.

Sign up for GitHub

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

Open
heptaflar wants to merge 4 commits into
base: 8.x
Choose a base branch
from
Open

Fix for clipping and rounding issues #267

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03e2aea
Fixes
heptaflar Feb 25, 2021
4120c32
New non-negative resampling type (decoration of existing types)
heptaflar Mar 2, 2021
5ba5a3f
Typo
heptaflar Mar 2, 2021
4da85bd
Reverted module version numbers
heptaflar Mar 2, 2021
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
86 changes: 82 additions & 4 deletions snap-gpf/src/main/java/org/esa/snap/core/gpf/internal/TileImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,17 @@ public double toGeoPhysical(double sample) {
return rasterDataNode.scale(sample);
}

private int toRaw(int sample) {
final double rawSample = rasterDataNode.scaleInverse(sample);
if (rawSample < -2147483648.0) {

@SabineEmbacher SabineEmbacher Feb 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which miracle Number ist this?
Why not
if (rawSample < Integer.MIN_VALUE)

return Integer.MIN_VALUE;
}
if (rawSample > 2147483647.0) {

@SabineEmbacher SabineEmbacher Feb 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same as above

return Integer.MAX_VALUE;
}
return (int) Math.round(rawSample);
}

@Override
public float toRaw(float sample) {
return (float) rasterDataNode.scaleInverse(sample);
Expand Down Expand Up @@ -492,7 +503,6 @@ public int getSampleInt(int x, int y) {
int sample = raster.getSample(x, y, 0);
// handle unsigned data types, see also [BEAM-1147] (nf - 20100527)
if (signedByte) {
//noinspection SillyAssignment
sample = (byte) sample;
}
if (scaled) {
Expand All @@ -504,17 +514,39 @@ public int getSampleInt(int x, int y) {
@Override
public void setSample(int x, int y, int sample) {
if (scaled) {
sample = (int) Math.floor(toRaw((double) sample) + 0.5);
sample = toRaw(sample);
}
switch (rasterDataNode.getDataType()) {
case ProductData.TYPE_INT8:
sample = clipOrRound(sample, -128, 127);
break;
case ProductData.TYPE_INT16:
sample = clipOrRound(sample, -32768, 32767);
break;
case ProductData.TYPE_UINT8:
sample = clipOrRound(sample, 0, 255);
break;
case ProductData.TYPE_UINT16:
sample = clipOrRound(sample, 0, 65535);
break;
}
writableRaster.setSample(x, y, 0, sample);
}

private int clipOrRound(int sample, int lowerBound, int upperBound) {

@SabineEmbacher SabineEmbacher Feb 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only a clip without round

if (sample < lowerBound) {
sample = lowerBound;
} else if (sample > upperBound) {
sample = upperBound;
}
return sample;
}

@Override
public float getSampleFloat(int x, int y) {
float sample = raster.getSampleFloat(x, y, 0);
// handle unsigned data types, see also [BEAM-1147] (nf - 20100527)
if (signedByte) {
//noinspection SillyAssignment
sample = (byte) sample;
}
if (scaled) {
Expand All @@ -528,16 +560,38 @@ public void setSample(int x, int y, float sample) {
if (scaled) {
sample = toRaw(sample);
}
switch (rasterDataNode.getDataType()) {
case ProductData.TYPE_INT8:
sample = clipOrRound(sample, -128.0f, 127.0f);
break;
case ProductData.TYPE_INT16:
sample = clipOrRound(sample, -32768.0f, 32767.0f);
break;
case ProductData.TYPE_UINT8:
sample = clipOrRound(sample, 0.0f, 255.0f);
break;
case ProductData.TYPE_UINT16:
sample = clipOrRound(sample, 0.0f, 65535.0f);
break;
}
writableRaster.setSample(x, y, 0, sample);
}

private float clipOrRound(float sample, float lowerBound, float upperBound) {
if (sample < lowerBound) {
return lowerBound;
}
if (sample > upperBound) {
return upperBound;
}
return (float) Math.rint(sample);
}

@Override
public double getSampleDouble(int x, int y) {
double sample = raster.getSampleDouble(x, y, 0);
// handle unsigned data types, see also [BEAM-1147] (nf - 20100527)
if (signedByte) {
//noinspection SillyAssignment
sample = (byte) sample;
}
if (scaled) {
Expand All @@ -551,9 +605,33 @@ public void setSample(int x, int y, double sample) {
if (scaled) {
sample = toRaw(sample);
}
switch (rasterDataNode.getDataType()) {

@SabineEmbacher SabineEmbacher Feb 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If raw data run into a clipping there must be something wrong with geopysical data because they run outside the covered valid value range, defined by raw data type and scaling. In this case clipping the raw data value then turns a wrong geophysical value into a valid value.
This can not be right.

Clipping or other types of handling values outside the range should be done at the place where the TileImpl is used, because only the context knows how to handle values outside the range.

Clipping then should be done that way:
First calculate the geopysical clipping bound by scaling the raw min and raw max value (take care if signed or unsigned), then clip the geopysical.
Up to here al computations outside of TileImpl.
Then transform to raw and round to prevent the cast error.

An integer cast is neither a floor nor a round nor a cail operation.
A cast on a positive double value is equal to a floor operation.
But on negative values it is equal to a cail operation.
It changes the direction of the value change for negative numbers.

@heptaflar heptaflar Feb 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree.

  1. A geophysical value that is outside the range of the raw data type is not invalid. It is just too large to be stored. It is better to clip it than to pass it uncontrolled.
  2. Casting of a double or float to a short or byte yields rather weird results. Cast of a positive double or float to a short or byte is not equal to a floor operation. Test it yourself.
  3. A scientist developing an operator probably does not want to bother with clipping and data types. Often the data type is predetermined (from external data) and not always the same.
  4. The API documentation of setSample states that the values are clipped, which they are not.
  5. A scientist's expectation probably is: he ensures that numbers (double or float) are calculated correctly. How these values are encoded into an image or a file and decoded again is business of the software framework. Encoding may not be lossless. But if something cannot be encoded without loss, the software framework should handle it in a smart and easily predictible way that is as good an approximation to the original information as possible.

@SabineEmbacher SabineEmbacher Feb 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. A geophysical value that is outside the range of the raw data type is not invalid. It is just too large to be stored. It is better to clip it than to pass it uncontrolled.

Thats exactly what I meant.
Values are invalid from the point of view of "which data can be saved" not from the Point of view, does the value make sence from the scientific poit of view of valid data range.

If the value is in a scientific valid range, but can not be saved, then the configuration of the store must be wrong. Then either the raw data type or the scaling must be wrong.

If a value is a scientific valid value, a raw clipping changes this scientific valid value. Does this make sence?

Ideally a raw date store should not manipulate data. Also a raw data store shall not know anything of the valid scientific value range. A raw data store shall only be able to tore store all the scientivic valid data of a cerain use case. For this a raw data store has to be configured the right way. Then scientific valid data can be stored using the scaling but without manipulation.

case ProductData.TYPE_INT8:
sample = clipOrRound(sample, -128.0, 127.0);
break;
case ProductData.TYPE_INT16:
sample = clipOrRound(sample, -32768.0, 32767.0);
break;
case ProductData.TYPE_UINT8:
sample = clipOrRound(sample, 0.0, 255.0);
break;
case ProductData.TYPE_UINT16:
sample = clipOrRound(sample, 0.0, 65535.0);
break;
}
writableRaster.setSample(x, y, 0, sample);
}

private double clipOrRound(double sample, double lowerBound, double upperBound) {
if (sample < lowerBound) {
return lowerBound;
}
if (sample > upperBound) {
return upperBound;
}
return Math.rint(sample);
}

@Override
public boolean getSampleBit(int x, int y, int bitIndex) {
long sample = raster.getSample(x, y, 0);
Expand Down
170 changes: 168 additions & 2 deletions snap-gpf/src/test/java/org/esa/snap/core/gpf/internal/TileImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,174 @@ public void testSetSamples() {
samples = tile.getSamplesDouble();
assertNotNull(samples);
assertEquals(N, samples.length);
assertEquals(10.0, samples[0], 1.0e-10);
assertEquals(10.0, samples[N - 1], 1.0e-10);
assertEquals(12.5, samples[0], 0.0);
assertEquals(12.5, samples[N - 1], 0.0);
}

@Test
public void testSetSamples_Double_INT8() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_INT8, factor);

tile.setSample(0, 0, 1.0);
assertEquals(1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

tile.setSample(0, 0, -1.0);
assertEquals(-1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

final double upperBound = 127.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, tooBig);
assertEquals(upperBound * factor, tile.getSampleDouble(0,0), 0.0);

final double negativeTooBig = -tooBig;
final double lowerBound = -upperBound - 1.0;
tile.setSample(0, 0, negativeTooBig);
assertEquals(lowerBound * factor, tile.getSampleDouble(0,0), 0.0);
}

@Test
public void testSetSamples_Double_INT16() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_INT16, factor);

tile.setSample(0, 0, 1.0);
assertEquals(1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

tile.setSample(0, 0, -1.0);
assertEquals(-1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

final double upperBound = 32767.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, tooBig);
assertEquals(upperBound * factor, tile.getSampleDouble(0,0), 0.0);

final double lowerBound = -upperBound - 1.0;
tile.setSample(0, 0, -tooBig);
assertEquals(lowerBound * factor, tile.getSampleDouble(0,0), 0.0);
}

@Test
public void testSetSamples_Double_UINT8() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_UINT8, factor);

tile.setSample(0, 0, 1.0);
assertEquals(1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

tile.setSample(0, 0, 0.0);
assertEquals(0.0, tile.getSampleDouble(0,0), 0.0);

final double upperBound = 255.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, tooBig);
assertEquals(upperBound * factor, tile.getSampleDouble(0,0), 0.0);

tile.setSample(0, 0, -1.0);
assertEquals(0.0, tile.getSampleDouble(0,0), 0.0);
}

@Test
public void testSetSamples_Double_UINT16() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_UINT16, factor);

tile.setSample(0, 0, 1.0);
assertEquals(1.0, tile.getSampleDouble(0,0), 0.015 / 2.0);

tile.setSample(0, 0, 0.0);
assertEquals(0.0, tile.getSampleDouble(0,0), 0.0);

final double upperBound = 65535.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, tooBig);
assertEquals(upperBound * factor, tile.getSampleDouble(0,0), 0.0);

tile.setSample(0, 0, -1.0);
assertEquals(0.0, tile.getSampleDouble(0,0), 0.0);
}

@Test
public void testSetSamples_Float_INT8() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_INT8, factor);

tile.setSample(0, 0, 1.0f);
assertEquals(1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

tile.setSample(0, 0, -1.0f);
assertEquals(-1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

final double upperBound = 127.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, (float) tooBig);
assertEquals((float) (upperBound * factor), tile.getSampleFloat(0,0), 0.0);

final double negativeTooBig = -tooBig;
final double lowerBound = -upperBound - 1.0;
tile.setSample(0, 0, (float) negativeTooBig);
assertEquals((float) (lowerBound * factor), tile.getSampleFloat(0,0), 0.0);
}

@Test
public void testSetSamples_Float_INT16() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_INT16, factor);

tile.setSample(0, 0, 1.0f);
assertEquals(1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

tile.setSample(0, 0, -1.0f);
assertEquals(-1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

final double upperBound = 32767.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, (float) tooBig);
assertEquals((float) (upperBound * factor), tile.getSampleFloat(0,0), 0.0);

final double lowerBound = -upperBound - 1.0;
tile.setSample(0, 0, (float) -tooBig);
assertEquals((float) (lowerBound * factor), tile.getSampleFloat(0,0), 0.0);
}

@Test
public void testSetSamples_Float_UINT8() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_UINT8, factor);

tile.setSample(0, 0, 1.0f);
assertEquals(1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

tile.setSample(0, 0, 0.0f);
assertEquals(0.0, tile.getSampleFloat(0,0), 0.0);

final double upperBound = 255.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, (float) tooBig);
assertEquals((float) (upperBound * factor), tile.getSampleFloat(0,0), 0.0);

tile.setSample(0, 0, -1.0f);
assertEquals(0.0, tile.getSampleFloat(0,0), 0.0);
}

@Test
public void testSetSamples_Float_UINT16() {
final double factor = 0.015;
final Tile tile = createScaledTile(ProductData.TYPE_UINT16, factor);

tile.setSample(0, 0, 1.0f);
assertEquals(1.0, tile.getSampleFloat(0,0), 0.015 / 2.0);

tile.setSample(0, 0, 0.0f);
assertEquals(0.0, tile.getSampleFloat(0,0), 0.0);

final double upperBound = 65535.0;
final double tooBig = upperBound * (factor + 0.001);
tile.setSample(0, 0, (float) tooBig);
assertEquals((float) (upperBound * factor), tile.getSampleFloat(0,0), 0.0);

tile.setSample(0, 0, -1.0f);
assertEquals(0.0, tile.getSampleFloat(0,0), 0.0);
}

static Tile createRawTile(int type) {
Expand Down