Fix min/max String Exception bug #577
Conversation
| // unclamped String | ||
| final String asciiUnclamped = (String) ops.run(DefaultASCII.class, img); | ||
|
|
||
| assertTrue(asciiUnclamped != ascii); |
There was a problem hiding this comment.
When comparing strings, use equals instead of !=. The latter compares reference equality, which is not what you want to test here. Better in this case: use assertEquals(asciiUnclamped, ascii). And if you do want to compare reference equality, you can use assertSame for that.
| for (int j = 0; j < width; j++) { | ||
| assertTrue(ascii.charAt(i * (width + 1) + j) == CHARS.charAt(i)); | ||
| } | ||
| assertTrue(ascii.charAt(i * (width + 1) + width) == '\n'); |
| final String ascii = (String) ops.run(DefaultASCII.class, img, min, max); | ||
| for (int i = 0; i < len; i++) { | ||
| for (int j = 0; j < width; j++) { | ||
| assertTrue(ascii.charAt(i * (width + 1) + j) == CHARS.charAt(i)); |
| @@ -115,6 +117,8 @@ public String calculate(final IterableInterval<T> input) { | |||
| // normalized = (value - min) / (max - min) | |||
| tmp.set(cursor.get()); | |||
| tmp.sub(min); | |||
| // NB: if a value is below min we set tmp to zero. | |||
| if (tmp.compareTo(zero) < 0) tmp.setZero(); | |||
| final double normalized = tmp.getRealDouble() / span.getRealDouble(); | |||
There was a problem hiding this comment.
As long as we are bending over backwards to use the ImgLib2 math APIs... why don't we use tmp.div(span), and only afterward call getRealDouble on the result?
There was a problem hiding this comment.
Actually, this will not work if the type does not support decimal values, since we are normalizing the data. I will fix this Wednesday morning.
| @@ -115,6 +117,8 @@ public String calculate(final IterableInterval<T> input) { | |||
| // normalized = (value - min) / (max - min) | |||
| tmp.set(cursor.get()); | |||
| tmp.sub(min); | |||
| // NB: if a value is below min we set tmp to zero. | |||
| if (tmp.compareTo(zero) < 0) tmp.setZero(); | |||
There was a problem hiding this comment.
It is inconsistent to protect against <0 normalized values here, but then protect against >=1 normalized values inside the charAt call below. I would prefer to do both checks the same way: by clamping charIndex to [0, charLen). Then you do not need any contortions with a special zero variable etc.
|
@ctrueden I reorganized the math within |
|
@ctrueden after pushing the changes that you suggested I got test failures. This is due to the fact that we cannot simply use |
|
I have a thought. This op is nothing else than a convert/scale/dither to the right destination type, followed by a direct mapping of that type's values to character symbols. We should lean on another op foe the first part, and solve the scaling conversion problem really well across a wide variety of types in that helper op. |
|
@ctrueden I took your advice and leaned on some helper |
|
@ctrueden I rebased this over |
When you provide an image of a type that can be negative, the Op can attempt to access the string of valid ascii image characters at a negative index, throwing an error. This bug was fixed and a test was added with the code that threw an error before, as well as making sure that the clamped ascii image is not the same as the unclamped image.
Note: if the RealType of the image is not a type supporting floating point numbers, then div(span) on that type would cause a rounding error. This is the reason that normalized has to call getRealDouble() twice, although it seems it would be nicer if we used the Imglib2 Type Math.
When you provide an image of a type that can be negative, the Op can attempt to access the string of valid ascii image characters at a negative index, throwing an error. This PR fixes the bug and adds a test containing the code that threw an error before, as well as making sure that the clamped ascii image is not the same as the unclamped image.