| Message ID | 20190110233401.4844-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | f477ee3e8956cd5a83282033e93c7673f40bd598 |
| Headers | show |
On 10/01/2019 23:34, James Almer wrote: > - if (!float_near_abs_eps(cdst[i], odst[i], FLT_EPSILON)) { > + if (!float_near_abs_eps(cdst[i], odst[i], 6.2e-05)) { Can you elaborate a bit more on this? FLT_EPSILON is used correctly here as far as I can tell, and it is not clear why it fails on x86_32, and why we should choose an arbitrary unportable number instead (who knows if it explodes on weird systems). - Derek
On Fri, Jan 11, 2019 at 2:12 PM Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > > On 10/01/2019 23:34, James Almer wrote: > > - if (!float_near_abs_eps(cdst[i], odst[i], FLT_EPSILON)) { > > + if (!float_near_abs_eps(cdst[i], odst[i], 6.2e-05)) { > > Can you elaborate a bit more on this? FLT_EPSILON is used correctly here > as far as I can tell, and it is not clear why it fails on x86_32, and why > we should choose an arbitrary unportable number instead (who knows if it > explodes on weird systems). > Because the computation accumulates more inaccuarcy then FLT_EPSILON allows for. That value is really not of that great use. If you have two accurate numbers and do one calculation, it may work, but if you do a whole bunch of them, the error accumulates and eventually gets bigger then FLT_EPSILON. x86_32 floating point is for $reasons a tad bit less accurate then on x86_64, for example, resulting in the test failing. We have some other float tests that do (or used to) fail sporadically due to inaccuracy problems, which sometimes where fixed by similar means - or multiplifying FLT_EPSILON to make it bigger. - Hendrik
On 11/01/2019 13:28, Hendrik Leppkes wrote: > Because the computation accumulates more inaccuarcy then FLT_EPSILON > allows for. That value is really not of that great use. If you have > two accurate numbers and do one calculation, it may work, but if you > do a whole bunch of them, the error accumulates and eventually gets > bigger then FLT_EPSILON. > x86_32 floating point is for $reasons a tad bit less accurate then on > x86_64, for example, resulting in the test failing. We have some other > float tests that do (or used to) fail sporadically due to inaccuracy > problems, which sometimes where fixed by similar means - or > multiplifying FLT_EPSILON to make it bigger. OK. Two things: 1. That should be in the commit messages. 2. Would some multiple of FLT_EPSILON make more sense? - Derek
On 1/11/2019 12:17 PM, Derek Buitenhuis wrote: > On 11/01/2019 13:28, Hendrik Leppkes wrote: >> Because the computation accumulates more inaccuarcy then FLT_EPSILON >> allows for. That value is really not of that great use. If you have >> two accurate numbers and do one calculation, it may work, but if you >> do a whole bunch of them, the error accumulates and eventually gets >> bigger then FLT_EPSILON. >> x86_32 floating point is for $reasons a tad bit less accurate then on >> x86_64, for example, resulting in the test failing. We have some other >> float tests that do (or used to) fail sporadically due to inaccuracy >> problems, which sometimes where fixed by similar means - or >> multiplifying FLT_EPSILON to make it bigger. > > OK. > > Two things: > > 1. That should be in the commit messages. > 2. Would some multiple of FLT_EPSILON make more sense? Michael suggested 1000*FLT_EPSILON but IMO that's too big and may hide errors in future implementations. The value i used is the smallest value i found that didn't fail after several runs. 6.1e-05 for example fails. > > - Derek > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 13/01/2019 02:44, James Almer wrote: > Michael suggested 1000*FLT_EPSILON but IMO that's too big and may hide > errors in future implementations. > The value i used is the smallest value i found that didn't fail after > several runs. 6.1e-05 for example fails. I figured that's how it was chosen (though not documented). I thought there be a way to calculate it properly instead of empirically. - Derek
On 1/13/2019 11:32 AM, Derek Buitenhuis wrote: > On 13/01/2019 02:44, James Almer wrote: >> Michael suggested 1000*FLT_EPSILON but IMO that's too big and may hide >> errors in future implementations. >> The value i used is the smallest value i found that didn't fail after >> several runs. 6.1e-05 for example fails. > > I figured that's how it was chosen (though not documented). I thought there > be a way to calculate it properly instead of empirically. If there is, i don't know it. Float based fate tests have been fine tuned before when different architectures proved a certain stddev value was not lax enough, so this one could be increased if needed as well, but if you prefer i can use a big enough multiple of FLT_EPSILON instead.
On 13/01/2019 15:45, James Almer wrote: > If there is, i don't know it. > > Float based fate tests have been fine tuned before when different > architectures proved a certain stddev value was not lax enough, so this > one could be increased if needed as well, but if you prefer i can use a > big enough multiple of FLT_EPSILON instead. Don't really have a strong opinion on it, no. - Derek
On 1/13/2019 12:55 PM, Derek Buitenhuis wrote: > On 13/01/2019 15:45, James Almer wrote: >> If there is, i don't know it. >> >> Float based fate tests have been fine tuned before when different >> architectures proved a certain stddev value was not lax enough, so this >> one could be increased if needed as well, but if you prefer i can use a >> big enough multiple of FLT_EPSILON instead. > > Don't really have a strong opinion on it, no. > > - Derek Pushes as is then. Thanks.
On 13/01/2019 18:01, James Almer wrote:
> Pushes as is then. Thanks.
Er.
You didn't add the actual description of the problem/fix
to the commit message.
- Derek
On 1/13/2019 3:18 PM, Derek Buitenhuis wrote: > On 13/01/2019 18:01, James Almer wrote: >> Pushes as is then. Thanks. > > Er. > > You didn't add the actual description of the problem/fix > to the commit message. I thought i had amended the patch, but guess not... I can revert and reapply with the amended commit message if you want, but it will kinda litter the tree for not a lot of gain. Then again, the tree is already a mess with all the merges.
On 13/01/2019 18:24, James Almer wrote: > I thought i had amended the patch, but guess not... > > I can revert and reapply with the amended commit message if you want, > but it will kinda litter the tree for not a lot of gain. Then again, the > tree is already a mess with all the merges. Not worth it, no. - Derek
diff --git a/tests/checkasm/af_afir.c b/tests/checkasm/af_afir.c index 54e2f68d6c..e3fb76e8e0 100644 --- a/tests/checkasm/af_afir.c +++ b/tests/checkasm/af_afir.c @@ -53,7 +53,7 @@ static void test_fcmul_add(const float *src0, const float *src1, const float *sr call_ref(cdst, src1, src2, LEN); call_new(odst, src1, src2, LEN); for (i = 0; i <= LEN*2; i++) { - if (!float_near_abs_eps(cdst[i], odst[i], FLT_EPSILON)) { + if (!float_near_abs_eps(cdst[i], odst[i], 6.2e-05)) { fprintf(stderr, "%d: %- .12f - %- .12f = % .12g\n", i, cdst[i], odst[i], cdst[i] - odst[i]); fail();
Should fix failures on x86_32. Signed-off-by: James Almer <jamrial@gmail.com> --- tests/checkasm/af_afir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)