Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

estimateQuadraticCurveLength test failure #41

Open
nckx opened this issue Jul 20, 2022 · 9 comments
Open

estimateQuadraticCurveLength test failure #41

nckx opened this issue Jul 20, 2022 · 9 comments

Comments

@nckx
Copy link

nckx commented Jul 20, 2022

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?

@justvanrossum
Copy link
Contributor

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

@nckx
Copy link
Author

nckx commented Jul 20, 2022

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…

@justvanrossum
Copy link
Contributor

Maybe we just got lucky...

A PR would be great.

@nckx
Copy link
Author

nckx commented Jul 20, 2022

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…

@justvanrossum
Copy link
Contributor

Something in this direction should work:

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

@emixa-d
Copy link

emixa-d commented Jul 24, 2022

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
Copy link

emixa-d commented Jul 24, 2022

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.

@nckx
Copy link
Author

nckx commented Jul 24, 2022

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…?

@typemytype
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants