snap-engine icon indicating copy to clipboard operation
snap-engine copied to clipboard

Fix for clipping and rounding issues

Open heptaflar opened this issue 4 years ago • 6 comments

This pull request fixes (some) clipping and rounding issues due to (in my opinion) wrong implementations of setSample(int x, int y, double / float / int sample) in TileImpl. The original implementation did not

  • Clip the argument (or inverse-scaled argument) to the value range of the underlying data type as stated in the API documentation
  • Round floating point values to the nearest integer value

The first point leads to unpredictible behaviour (may be predictable for a Java software developer, but not for a scientist who wants to code an operator). The second points leads to a systematic bias when written data are read again.

For example, as a consequence of these issues, the collocation operator produces extremely high (and wrong) signals from almost zero signal for short data types when higher than bilinear interpolation (like bicubic interpolation) is requested.

Please review my changes. Note that they highlight the problems and are merely suggestions how to possibly solve them (in part).

heptaflar avatar Feb 25 '21 17:02 heptaflar

Feel free to rename methods and extract constants as you like. I regard it as good programming style to compare variables of the same type only, but you may care about this or not. My only concern here is clipping and rounding.

heptaflar avatar Feb 26 '21 09:02 heptaflar

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

TileImpl is not only used in CollocationOp but also in many other contexts.

SabineEmbacher avatar Feb 26 '21 10:02 SabineEmbacher

If new values are to be expected only slightly outside the valid range (defined by data type and scaling factors) because, for example, one has decided to use an interpolation type that is not a linear interpolation for a resampling, then the problem can also be circumvented by slightly reducing the scaling factor in the target raster. Then overshoots from the interpolation fit back into the raw data area.

SabineEmbacher avatar Feb 26 '21 10:02 SabineEmbacher

Well, the API documentation states that values are clipped. But they are not. The purpose of setSample(x, y, double value) is to encode the value into the underlying data type, which may be different from double. Encoding involves clipping. So the API documentation makes complete sense to me.

Your suggestions work technically but do not solve the encoding / decoding problem for raster data types, which are given and whose properties must not be changed.

heptaflar avatar Feb 26 '21 11:02 heptaflar

@Test
public void testCasts() {
    // all these tests fail, prior clipping is needed
    Assert.assertEquals(Short.MAX_VALUE, (short) 1.0E6);
    Assert.assertEquals(Short.MIN_VALUE, (short) -1.0E6);
    Assert.assertEquals(Byte.MAX_VALUE, (byte) 1.0E3);
    Assert.assertEquals(Byte.MIN_VALUE, (byte) -1.0E3);

    // these tests do not fail, no clipping is needed
    Assert.assertEquals(Integer.MAX_VALUE, (int) 1.0E27);
    Assert.assertEquals(Integer.MIN_VALUE, (int) -1.0E27);

    // this test fails, rounding is needed (otherwise encoding / decoding would produce a bias)
    Assert.assertEquals(2.0, (int) 1.999);
}

heptaflar avatar Feb 26 '21 12:02 heptaflar

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jun 14 '23 08:06 CLAassistant