[FFmpeg-devel] checkasm/af_afir: relax the max allowed absolute difference

Submitted by James Almer on Jan. 10, 2019, 11:34 p.m.

Details

Message ID 20190110233401.4844-1-jamrial@gmail.com
State Accepted
Commit f477ee3e8956cd5a83282033e93c7673f40bd598
Headers show

Commit Message

James Almer Jan. 10, 2019, 11:34 p.m.
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(-)

Comments

Derek Buitenhuis Jan. 11, 2019, 1:12 p.m.
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
Hendrik Leppkes Jan. 11, 2019, 1:28 p.m.
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
Derek Buitenhuis Jan. 11, 2019, 3:17 p.m.
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
James Almer Jan. 13, 2019, 2:44 a.m.
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
>
Derek Buitenhuis Jan. 13, 2019, 2:32 p.m.
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
James Almer Jan. 13, 2019, 3:45 p.m.
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.
Derek Buitenhuis Jan. 13, 2019, 3:55 p.m.
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
James Almer Jan. 13, 2019, 6:01 p.m.
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.
Derek Buitenhuis Jan. 13, 2019, 6:18 p.m.
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
James Almer Jan. 13, 2019, 6:24 p.m.
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.
Derek Buitenhuis Jan. 13, 2019, 6:27 p.m.
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

Patch hide | download patch | download mbox

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();