After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 734725 - Add fuzzy comparison macros
Add fuzzy comparison macros
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-08-13 15:54 UTC by Emmanuele Bassi (:ebassi)
Modified: 2018-05-24 16:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add fuzzy comparison macros (3.16 KB, patch)
2014-08-13 15:54 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
tests: Add fuzzy floating point comparison macro (5.80 KB, patch)
2015-08-31 19:29 UTC, Emmanuele Bassi (:ebassi)
reviewed Details | Review
Add fuzzy floating point comparison macro (4.11 KB, patch)
2017-02-08 17:35 UTC, Emmanuele Bassi (:ebassi)
none Details | Review
Add a macro for checking approximate values (2.19 KB, patch)
2017-02-08 17:36 UTC, Emmanuele Bassi (:ebassi)
none Details | Review

Description Emmanuele Bassi (:ebassi) 2014-08-13 15:54:00 UTC
There are at least two projects with equivalent macros or code that works to this effect:

  - Graphene
  - Clutter

and there's code in the GTK+ test suite that ought to really *not* use g_assert_cmpfloat() with '==' as a comparison operator.
Comment 1 Emmanuele Bassi (:ebassi) 2014-08-13 15:54:08 UTC
Created attachment 283299 [details] [review]
Add fuzzy comparison macros

Various projects using GTest end up re-implementing them, so it would be
nice to ship these macros with GLib itself.
Comment 2 Dan Winship 2014-08-13 21:28:30 UTC
(In reply to comment #0)
> and there's code in the GTK+ test suite that ought to really *not* use
> g_assert_cmpfloat() with '==' as a comparison operator.

And GLib too. See bug 722604. I pondered this a little then, but never actually wrote a patch.

Do you actually need the epsilon argument? You basically want to ask "are n1 and n2 equal/unequal within the bounds of floating-point representation", so can't you calculate an appropriate epsilon automatically based on the magnitude of n1 and n2?

And also, you don't need two macros; you can have a single g_assert_fuzzy_cmpfloat(n1, cmp, n2) macro that takes a comparison operator as an argument like the other g_assert_cmp* macros, by having the macro experiment with "cmp" to figure out what it is, and then handling each case separately:

    if ((0 cmp 1) && (1 cmp 0))
      {
        /* cmp is != */
        ...
      }
    else if ((0 cmp 1) && !(1 cmp 0))
      {
        /* cmp is < or <=, which are the same for our purposes */
        ...
      }
    else...


Given that, and if we don't need an explicit epsilon argument, would it make sense to just "fix" g_assert_cmpfloat() rather than introducing a separate fuzzy macro?
Comment 3 Emmanuele Bassi (:ebassi) 2014-08-13 21:36:56 UTC
(In reply to comment #2)
> (In reply to comment #0)
> > and there's code in the GTK+ test suite that ought to really *not* use
> > g_assert_cmpfloat() with '==' as a comparison operator.
> 
> And GLib too. See bug 722604. I pondered this a little then, but never actually
> wrote a patch.
> 
> Do you actually need the epsilon argument? You basically want to ask "are n1
> and n2 equal/unequal within the bounds of floating-point representation", so
> can't you calculate an appropriate epsilon automatically based on the magnitude
> of n1 and n2?

no, I really want the epsilon.

in some cases, I have public API that has an epsilon argument that I want to check, but in general not everything has to be matched to the representation of a floating point value.

for instance, it does not matter if the interpolated actor's width is within 0.00001 pixels of the target width: it's going to be invisible in any case, so I can truncate at an arbitrary precision that is not 1e-15.

another instance would be a RGBA tuple, where I can live with +/- 1 accuracy on a 65536 range of acceptable colors.

the original macro I wrote for graphene uses typeof(), which I guess it's a bit better than forcing a long double conversion, but I honestly don't know if typeof() is supported on every compiler we target in GLib.

> And also, you don't need two macros; you can have a single
> g_assert_fuzzy_cmpfloat(n1, cmp, n2) macro that takes a comparison operator as
> an argument like the other g_assert_cmp* macros, by having the macro experiment
> with "cmp" to figure out what it is, and then handling each case separately:
> 
>     if ((0 cmp 1) && (1 cmp 0))
>       {
>         /* cmp is != */
>         ...
>       }
>     else if ((0 cmp 1) && !(1 cmp 0))
>       {
>         /* cmp is < or <=, which are the same for our purposes */
>         ...
>       }
>     else...

I wanted to keep the macro simple; also, introducing a comparison operator with an epsilon value only makes sense for two cases: equality and non equality.
Comment 4 Allison Karlitskaya (desrt) 2014-08-18 13:50:24 UTC
fwiw, I think this function makes more sense with the epsilon than without it.  If we're talking about "are these things equal to within floating point representation" then imho we're really discussing a bug in the C compiler...
Comment 5 Dan Winship 2014-08-18 15:06:42 UTC
The case in bug 722604 may be a compiler bug, but floating point is inherently fuzzy, so if one codepath computes (x+y)+z and the other computes x+(y+z), they might get different answers.

(But yeah, you do need the epsilon. If nothing else, in some cases you don't get to compare the result until after it has passed through multiple floating point operations, so a small initial error may get multiplied.)
Comment 6 Dan Winship 2014-08-18 15:07:13 UTC
(In reply to comment #3)
> the original macro I wrote for graphene uses typeof(), which I guess it's a bit
> better than forcing a long double conversion, but I honestly don't know if
> typeof() is supported on every compiler we target in GLib.

It's not. It's a gnu-ism.
Comment 7 Richard Hughes 2015-08-03 15:45:10 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #0)
>   - Graphene
>   - Clutter

fwupd, colord and PackageKit too...
Comment 8 Emmanuele Bassi (:ebassi) 2015-08-31 19:29:23 UTC
Created attachment 310379 [details] [review]
tests: Add fuzzy floating point comparison macro

Add a test macro that allows comparing two floating point values for
equality within a certain tolerance.

This macro has been independently reimplemented by various projects, and
it's useful for testing inside GLib itself.
Comment 9 Matthias Clasen 2015-08-31 19:34:07 UTC
Review of attachment 310379 [details] [review]:

::: glib/gtestutils.c
@@ +586,3 @@
+ * ]|
+ *
+ * Since: 2.46

Not super-thrilled by the name, but ok.

A more substantial issue is the lack of comparison: compare

g_assert_cmpfloat (n1, ==, n2)
g_assert_cmpfloat_fuzzy (n1, n2, 0.01)

g_assert_cmpfloat (n1, <=, n2)
g_assert_cmpfloat_fuzzy  ?!
Comment 10 Dan Winship 2015-08-31 19:36:59 UTC
Comment on attachment 310379 [details] [review]
tests: Add fuzzy floating point comparison macro

>+++ b/glib/tests/testing.c
>@@ -68,6 +68,8 @@ test_assertions (void)
>   g_assert_cmphex (2, ==, 2);
>   g_assert_cmpfloat (3.3, !=, 7);
>   g_assert_cmpfloat (7, <=, 3 + 4);
>+  g_assert_cmpfloat_fuzzy (3.14, 3.15, 0.01);
>+  g_assert_cmpfloat_fuzzy (3.14159, 3.1416, 0.0001);

need failing tests too

>         g_printerr ("\t=> timer = %.6f = %d.%06ld (should be: 9.000000) (%.6f off)\n", elapsed, (int) elapsed, elapsed_usecs, ABS (elapsed - 9.));
>       g_assert_cmpfloat (elapsed, >, 8.8);
>       g_assert_cmpfloat (elapsed, <, 9.2);
>+      g_assert_cmpfloat_fuzzy (elapsed, 9.0, 0.5);

Shouldn't keep the existing non-fuzzy tests, but also, why are you changing the epsilon relative to the old tests?
Comment 11 Emmanuele Bassi (:ebassi) 2017-02-08 17:35:58 UTC
Created attachment 345253 [details] [review]
Add fuzzy floating point comparison macro

Add a test macro that allows comparing two floating point values for
equality within a certain tolerance.

This macro has been independently reimplemented by various projects:

 * Clutter
 * Graphene
 * colord
Comment 12 Emmanuele Bassi (:ebassi) 2017-02-08 17:36:04 UTC
Created attachment 345254 [details] [review]
Add a macro for checking approximate values

A macro like this is useful to avoid direct comparisons between floating
point values.
Comment 13 Emmanuele Bassi (:ebassi) 2017-02-08 17:41:08 UTC
The patch in attachment 345254 [details] [review] is a "nice to have" but should not be a blocker for the one in attachment 345253 [details] [review].
Comment 14 Emmanuele Bassi (:ebassi) 2017-02-08 17:45:17 UTC
(In reply to Matthias Clasen from comment #9)
> Review of attachment 310379 [details] [review] [review]:
> 
> ::: glib/gtestutils.c
> @@ +586,3 @@
> + * ]|
> + *
> + * Since: 2.46
> 
> Not super-thrilled by the name, but ok.

Changed to "cmpfloat_with_epsilon()'

> A more substantial issue is the lack of comparison: compare
> 
> g_assert_cmpfloat (n1, ==, n2)
> g_assert_cmpfloat_fuzzy (n1, n2, 0.01)
> 
> g_assert_cmpfloat (n1, <=, n2)
> g_assert_cmpfloat_fuzzy  ?!

There's not much of a point in doing order comparisons within an epsilon. To be quite honest, I've also never saw a `g_assert_cmpstr ("good", <=, "bad")` in the wild either.

Anyway, I'm not going to spend any more time on this than I already have. If somebody else wants to pick this up, feel free.
Comment 15 Debarshi Ray 2017-02-08 18:16:44 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #12)
> Created attachment 345254 [details] [review] [review]
> Add a macro for checking approximate values
> 
> A macro like this is useful to avoid direct comparisons between floating
> point values.

I have seen a copy of this in at least gegl, gnome-photos and shotwell.
Comment 16 GNOME Infrastructure Team 2018-05-24 16:55:00 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/914.