Wrong use of float instead of double?

[slaperche-zenly]

I was strolling around the codebase, and stumbled upon this:

https://github.com/uber/h3/blob/cd87201c1b0c3caf265f402378790b5737529c97/src/h3lib/lib/vec2d.c#L41-L55

Seeing that most, if not all, of the codebase use double for calculation, this looks like a mistake/bug to me (the use of float for t).

Or am I missing something that justify the use of float here?

From a quick testing using cellToBoundary the precision loss does affect, in some cases, the end result in a way that's above the EPSILON_RAD threshold.

[isaacbrodsky]

Thanks for pointing this out. Could you share the test case you used?

We had previously done a cleanup pass adjusting types to double (mostly from long double), which I agree probably should have included this.

[slaperche-zenly]

Could you share the test case you used?

Sure.

I've found difference greater than EPSILON_RAD for those 3 index.

H3 index 81083ffffffffff

Usingfloat t:

{
    (1.1052657257862937, 0.07003344631228453),
    (1.0801989074358653, 0.1508701234414947),
    (1.0740846031420808, 0.19339400131474488),
    (1.097637014975174,  0.2752693849343275),
    (1.1135338971807607, 0.30605127388841125),
    (1.157629102380263,  0.28682212024381437),
    (1.1755101204244496, 0.2585471190324795),
    (1.1775462077184973, 0.1418998206035864),
    (1.1696434396728306, 0.09144231782330836),
    (1.1261843175288389, 0.06457601677567326)
}

Using double t:

{
    (1.1052657257862937, 0.07003344631228453),
    (1.0801989082144292, 0.15087012116893278),
    (1.0740846031420808, 0.19339400131474488),
    (1.0976370143214977, 0.2752693824080523),
    (1.1135338971807607, 0.30605127388841125),
    (1.1576291010803654, 0.28682212086834635),
    (1.1755101204244496, 0.2585471190324795),
    (1.1775462077297505, 0.1418998240582856),
    (1.1696434396728306, 0.09144231782330836),
    (1.1261843188159308, 0.06457601749607167)
}

H3 index 830800fffffffff

Using float t:

{
    (1.126227637158774,  0.1675017850648683),
    (1.122430398667794,  0.1788942481098706),
    (1.1216131563684717, 0.18533258260457477),
    (1.1250336137398762, 0.19732301562588492),
    (1.1274119867370176, 0.2012235188354936),
    (1.1334149447671842, 0.1974554306780001),
    (1.1357391942104589, 0.1933094956222252),
    (1.1359913568989835, 0.17857329224768703),
    (1.1349938053375228, 0.17208944100469048),
    (1.1291353997907014, 0.16721083489048616)
}

Using double t:

{
    (1.126227637158774,  0.1675017850648683),
    (1.122430398781567,  0.17889424777346158),
    (1.1216131563684717, 0.18533258260457477),
    (1.125033613638905,  0.19732301526644672),
    (1.1274119867370176, 0.2012235188354936),
    (1.1334149445885897, 0.19745543079159478),
    (1.1357391942104589, 0.1933094956222252),
    (1.1359913568927131, 0.1785732926865311),
    (1.1349938053375228, 0.17208944100469048),
    (1.1291353999652103, 0.16721083503389478)
}

H3 index 85080003fffffff

Using float t:

{
    (1.1288078264453467, 0.18154866432937328),
    (1.1282611716551452, 0.18317142339796127),
    (1.1281454821307542, 0.18409908158167107),
    (1.1286353542935652, 0.18581747361938566),
    (1.128976942627859,  0.1863635044619012),
    (1.1298282272001177, 0.1858071042712941),
    (1.1301557790696835, 0.18521444606814486),
    (1.1301912579799267, 0.18314380621880488),
    (1.1300509912220875, 0.18223112612140624),
    (1.1292214073877282, 0.1815170150549581)
}

Using double t

{
    (1.1288078264453467, 0.18154866432937328),
    (1.1282611716714488, 0.1831714233496638),
    (1.1281454821307542, 0.18409908158167107),
    (1.1286353542789853, 0.18581747356812958),
    (1.128976942627859,  0.1863635044619012),
    (1.1298282271747533, 0.18580710428790348),
    (1.1301557790696835, 0.18521444606814486),
    (1.1301912579788937, 0.1831438062805102),
    (1.1300509912220875, 0.18223112612140624),
    (1.1292214074124503, 0.18151701507619888)
}

[nrabinowitz]

I added some tests based on cellsToLinkedMultiPolygon, which currently depends on comparing lat/lng coords from different cells, and I can confirm that changing to double causes some tests to pass that fail with float. I'll put up a PR now.

[slaperche-zenly]

We had previously done a cleanup pass adjusting types to double (mostly from long double), which I agree probably should have included this.

There is still a bunch of long double "hidden" in the codebase though: floating-point constants suffixed with L (e.g. M_SQRT7)

IIRC, in C:

• f suffix -> float
• no suffix -> double
• L suffix -> long double

But maybe it's fine. The extra-precision probably influence the results whether 80-bit float are available (Intel mainly, with the x87) or not, but less dramatically than the float issue here.