fontPens icon indicating copy to clipboard operation
fontPens copied to clipboard

estimateQuadraticCurveLength test failure

Open nckx opened this issue 3 years ago • 9 comments
trafficstars

Hi! We're receiving a sudden influx of reports on the following test failure on GNU/Linux:

=================================== FAILURES ===================================
___________ [doctest] fontPens.penTools.estimateQuadraticCurveLength ___________
155 
156     Estimate the length of this curve by iterating
157     through it and averaging the length of the flat bits.
158 
159     >>> estimateQuadraticCurveLength((0, 0), (0, 0), (0, 0)) # empty segment
160     0.0
161     >>> estimateQuadraticCurveLength((0, 0), (50, 0), (80, 0)) # collinear points
162     80.0
163     >>> estimateQuadraticCurveLength((0, 0), (50, 20), (100, 40)) # collinear points
Expected:
    107.70329614269009
Got:
    107.70329614269008

/tmp/guix-build-python-fontpens-0.2.4.drv-0/fontPens-0.2.4/Lib/fontPens/penTools.py:163: DocTestFailure
=============================== warnings summary ===============================
../../../gnu/store/zlg7s86frmvbyf27627h0lgy54lwrkl9-python-fonttools-4.28.5/lib/python3.9/site-packages/fontTools/misc/py23.py:13
  /gnu/store/zlg7s86frmvbyf27627h0lgy54lwrkl9-python-fonttools-4.28.5/lib/python3.9/site-packages/fontTools/misc/py23.py:13: DeprecationWarning: The py23 module has been deprecated and will be removed in a future release. Please update your code.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/warnings.html
=========================== short test summary info ============================
FAILED Lib/fontPens/penTools.py::fontPens.penTools.estimateQuadraticCurveLength
=================== 1 failed, 20 passed, 1 warning in 0.52s ====================

What's fun is it happens on some machines, and not others, and it's consistent: ‘good’ machines stay good, ‘bad’ machines remain bad.

I can't find any seemingly relevant recent changes. On a whim I tried downgrading the kernel on a ‘bad’ machine from 5.18 to 5.17, but no difference.

Thoughts? Has anybody else seen similar errors?

nckx avatar Jul 20 '22 11:07 nckx

We should probably round that result to some number of digits. These kinds of floating point differences are hard to avoid.

justvanrossum avatar Jul 20 '22 18:07 justvanrossum

Thanks for the swift reply, Just!

We should probably round that result to some number of digits.

Great: I was going to go with that as a ‘temporary’ downstream fix. Having it as an upstream patch is even better.

These kinds of floating point differences are hard to avoid.

So this was always a lurking bug? I'm still a bit confused about why we got 0 bug reports before this month, and at least 2 since. Like something changed, but I wouldn't know what…

nckx avatar Jul 20 '22 20:07 nckx

Maybe we just got lucky...

A PR would be great.

justvanrossum avatar Jul 20 '22 20:07 justvanrossum

It would, but I should point out I'm just a humble distro packager. I have no idea how to round numbers in Python.

I'll try looking it up, but don't expect high-quality code…

nckx avatar Jul 20 '22 20:07 nckx

Something in this direction should work:

>>> round(estimateQuadraticCurveLength((0, 0), (50, 20), (100, 40)), 11) # collinear points
107.70329614269

justvanrossum avatar Jul 21 '22 07:07 justvanrossum

I'm wondering if there's a difference in rounding between CPU models (on my computer, it passes):

processor       : 0
vendor_id       : AuthenticAMD
cpu family      : 23
model           : 104
model name      : AMD Ryzen 7 5700U with Radeon Graphics
stepping        : 1
microcode       : 0x8608103

emixa-d avatar Jul 24 '22 22:07 emixa-d

And here is a failing computer, from some helpful people at #guix IRC

processor    : 0
vendor_id    : GenuineIntel
cpu family   : 6
model        : 23
model name   : Intel(R) Core(TM)2 Duo CPU        P840 @ 2.26GHz
stepping     : 10
microcode    : 0xa0b

So perhaps an Intel / AMD difference, though not enough samples, and not that it matters I suppose.

emixa-d avatar Jul 24 '22 22:07 emixa-d

Something in this direction should work:

>>> round(estimateQuadraticCurveLength((0, 0), (50, 20), (100, 40)), 11) # collinear points
107.70329614269

Hmkay, but doesn't that reduce the ‘doc-’ quality of the ‘-string’? I'm more tempted to choose some less ‘controversial’ points that produce the same result everywhere, although that might be whack-a-mole…

[Edit: but if you think it doesn't, I'll happily submit a patch to that end — but then do we modify all such assertions?]

So perhaps an Intel / AMD difference, though not enough samples

Heh, oh dear. I thought the same thing, with the same amount of samples (1 of each).

Could it truly…?

nckx avatar Jul 24 '22 22:07 nckx

since fontTools is required anyhow these old fontPen.penTools should just mimic the fontTools callback for calculating curve length: see https://github.com/fonttools/fonttools/blob/main/Lib/fontTools/misc/bezierTools.py#L261

typemytype avatar Jul 25 '22 10:07 typemytype