Skip to content

Fix Floating Point Precision Issue in Lifting Surface Knot Symmetrization#287

Open
A-CGray wants to merge 4 commits intomainfrom
floatTol
Open

Fix Floating Point Precision Issue in Lifting Surface Knot Symmetrization#287
A-CGray wants to merge 4 commits intomainfrom
floatTol

Conversation

@A-CGray
Copy link
Copy Markdown
Member

@A-CGray A-CGray commented Mar 25, 2026

Purpose

Fixes a ValueError in pyGeo that occurs during the initialization of a liftingSurface when using airfoil sections with near-identical, but not bitwise-identical, knot distributions.

The core problem was a logical inconsistency in pygeo/pygeo/pyGeo.py between how unique knots were identified for the global knot vector and how they were checked for existence in individual cross-section curves.

  1. pyGeo builds a newKnots union by comparing knots with a tolerance of 1e-12. If a knot in Curve B is within 1e-12 of a knot in Curve A, it is considered "already present" and is not added to the global union.
  2. The code then loops through newKnots and checks if each knot exists in every curve using Python's not in operator:
    if newKnots[j] not in curves[i].t:
        curves[i].insertKnot(newKnots[j], mult[j])
  3. Because not in requires bitwise equality, a knot that was "close enough" ($< 10^{-12}$) to be rejected from the global union is still seen as "missing" by the not in check if the difference is, for example, $10^{-15}$.
  4. This causes an extra knot to be inserted into the curve, creating a mismatch between the number of control points and the knot vector length. When pyGeo later forces all curves to share curves[0].t, the "corrupted" curve triggers a ValueError in the underlying pyspline Fortran library.

The Fix

The not in check has been replaced with a tolerance-based search consistent with the rest of the knot-processing logic in pyGeo:

KNOT_TOL = 1e-12
.
.
.
found = False
for k_knot in curves[i].t:
    if abs(k_knot - newKnots[j]) < KNOT_TOL:
        found = True
        break
if not found:
    curves[i].insertKnot(newKnots[j], mult[j])

Expected time until merged

2 weeks

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Added a test using the files I originally encountered this issue using.

Checklist

  • I have run ruff check and ruff format to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@A-CGray A-CGray requested a review from a team as a code owner March 25, 2026 04:38
@A-CGray A-CGray requested review from anilyil, Copilot and sabakhshi and removed request for anilyil and sabakhshi March 25, 2026 04:38
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a ValueError during liftingSurface initialization by making knot-existence checks tolerance-based (consistent with how the global knot union is built), preventing erroneous extra knot insertions when knot values differ only at floating-point noise levels.

Changes:

  • Replaced exact-membership (not in) knot checks with a tolerance-based search using a shared KNOT_TOL.
  • Added a regression test case that exercises the near-identical-knot “clash” scenario alongside the original case.
  • Updated the regression reference file to include both test outputs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
pygeo/pyGeo.py Uses a consistent knot tolerance when checking whether to insert knots, preventing precision-triggered mismatches.
tests/reg_tests/test_pyGeo.py Splits the regression test into “original” and “clash” cases and trains/validates both.
tests/reg_tests/ref/test_pyGeo.ref Updates reference values to match the new two-case regression test output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@A-CGray A-CGray requested a review from sabakhshi March 26, 2026 19:22
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

Successfully merging this pull request may close these issues.

2 participants