FloatCounter test and possible corrections #7535
Conversation
| * | ||
| * else if (Float.isInfinite(value)) throw new RuntimeException("value is infinite"); else if | ||
| * (value==Float.MAX_VALUE||value==-Float.MAX_VALUE) throw new RuntimeException("value is too big and " + | ||
| * "there are risks that the next results will be different than expected"); |
There was a problem hiding this comment.
I do not like the idea of throwing an exception for crazy values. Someone might want to do that, or there might be some scenario where a game ends up at MAX_VALUE and now it'll crash (which is worse than just behaving incorrectly).
There was a problem hiding this comment.
should i keep the NaN exception though ?
There was a problem hiding this comment.
or maybe I should avoid having both POSITIVE_INFINITY and - NEGATIVE_INFINITY ? that's the issue I tested where the total was then NaN
There was a problem hiding this comment.
I wouldn't throw an exception over putting a NaN value for much the same reason. Maybe we should return early on NaN to avoid screwing up the total/average. I'd be interested to hear what someone else thinks.
Infinities and other extreme values will always cause problems.
There was a problem hiding this comment.
so like adding the NaN to the values but not to the total or average then ?
There was a problem hiding this comment.
I haven't inspected it but I reckon NaN would break WindowedMean.
There was a problem hiding this comment.
hey so should I keep FloatCounter as it is and rewrite the tests for it to take into account NaN etc ?
There was a problem hiding this comment.
If you want to. I'm still not that sure on any of this; would like to see if someone who actually uses FloatCounter has thoughts on the matter.
There was a problem hiding this comment.
okay, I can still wait for someone to confirm how it works
There was a problem hiding this comment.
Imagine if we checked for odd float values for every method that accepted floats -- it isn't reasonable to guard against unless there are some special requirements.
| counter1.put(7E18f); | ||
| Assert.assertEquals(1750000000000002519.00000000000000000000000000000000000002938735875f, counter1.average, 0); | ||
| counter1.put(5f); | ||
| Assert.assertEquals(1400000000000002013.80000000000000000000000000000000000002350988700f, counter1.mean.getMean(), 0); |
There was a problem hiding this comment.
This looks like you've hardcoded values that will pass the test. Consider using MathUtils#isEqual.
| counter2.put(Float.NEGATIVE_INFINITY); | ||
| /* | ||
| * After implementing this next test, I realized that putting POSITIVE_INFINITY then NEGATIVE_INFINITY, the total was not a | ||
| * number (same as before, should this be prohibited ?) |
There was a problem hiding this comment.
Consider marking your pull request as draft if it contains questions, since this doesn't look ready to be merged.
There was a problem hiding this comment.
also done thank you
I have worked on the com.badlogic.gdx.math FloatCounter class. I just wanted to add some tests to find out the limits of the counter and I am thus suggesting a few modifications to the class to avoid big errors with the large numbers (I think this could be interesting, since this class isn't meant for such usages, to prevent them from happening).