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

Make geometry spline interpolation mesh independent #435

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

A-CGray
Copy link
Member

@A-CGray A-CGray commented Jul 17, 2024

Purpose

Problem

OpenAeroStruct uses b-splines to interpolate geometric design variable values onto the mesh (either at mesh nodes or panel mid-points), these b-splines are defined over a coordinate range of 0 to 1, so it is necessary to map the spanwise coordinates of the mesh to the same range. In other words, we need to define where on the spline each mesh point interpolates it's design variable value from.

Currently, for all DVs that use this interpolation, we simply linearly space these interpolation points from 0 to 1. This works fine if your mesh has constant spanwise spacing but otherwise produces unintuitive results, it also means that for the same design variable values, changing the spanwise mesh distribution significantly changes the wing geometry.

Additionally, this approach means that for the t_over_c variable, the b-spline used for interpolation technically spans not from the root to the tip but from the centre of the first panel to the centre of the last panel. This means that, even if you keep a constant spanwise mesh spacing, the interpolation of t_over_c values will change depending on how many panels you use along the span.

Fix

This PR fixes the above issue by computing the points on the b-spline to interpolate from based on the actual mesh coordinates. This means that the wing geometry no longer depends on the spanwise mesh distribution, and the t_over_c interpolation no longer depends on the number of spanwise mesh points.

Example

In the case below I use the standard rectangular wing mesh and control x-shear and chord with 2 control points, one at the tip and one at the root. This should result in the chord length and x-shear being linearly interpolated on the wing, but with the current implementation this is not the case if you use cosine spanwise spacing (shown in blue). The Orange mesh is the geometry produced with the changes in this PR and is as you'd expect.

image

Expected time until merged

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

A few of the tests currently fail because this PR slightly alters how t_over_c is interpolated which slightly changes the optimized reference values we test against, if you're happy with these changes I will update the reference values.

Checklist

  • I have run flake8 and black 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 July 17, 2024 15:17
@A-CGray A-CGray requested review from lamkina, bernardopacini and kanekosh and removed request for bernardopacini July 17, 2024 15:17
Copy link
Contributor

@sabakhshi sabakhshi left a comment

Choose a reason for hiding this comment

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

Long standing bug fixed. Changes are minimal for the Geometry group but the tests still need to be updated as mentioned.

@kanekosh
Copy link
Contributor

This is great. I remember facing this bug a while ago and ended up using a custom geometry component for my project (bad practice, sorry!). Yes please update the test reference values.

Copy link

codecov bot commented Jul 18, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.94%. Comparing base (d788e11) to head (7921cad).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
openaerostruct/utils/testing.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   93.92%   93.94%   +0.02%     
==========================================
  Files         104      105       +1     
  Lines        6472     6478       +6     
==========================================
+ Hits         6079     6086       +7     
+ Misses        393      392       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@A-CGray
Copy link
Member Author

A-CGray commented Jul 18, 2024

This is great. I remember facing this bug a while ago and ended up using a custom geometry component for my project (bad practice, sorry!). Yes please update the test reference values.

Done, I realised there were some other places using B-spline components with the same issue so fixed those too

@A-CGray A-CGray requested a review from sabakhshi July 18, 2024 23:27
@A-CGray
Copy link
Member Author

A-CGray commented Jul 18, 2024

@sabakhshi probably better if you review this, not sure @lamkina is a particularly prolific OAS user

Copy link
Contributor

@sabakhshi sabakhshi left a comment

Choose a reason for hiding this comment

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

Same as the my previous comments. I approve.

@lamkina lamkina removed their request for review July 19, 2024 16:28
@lamkina
Copy link
Contributor

lamkina commented Jul 19, 2024

I removed myself as a reviewer, but my overall comment is the B-Spline corrections make sense so as long as the tests are passing this should be all set.

@lamkina
Copy link
Contributor

lamkina commented Jul 19, 2024

I'll leave it up to @kanekosh to merge as he is the senior maintainer here.

@A-CGray
Copy link
Member Author

A-CGray commented Jul 19, 2024

@sabakhshi or @kanekosh can one of you merge this?

@kanekosh kanekosh merged commit 9c123a3 into mdolab:main Jul 19, 2024
9 checks passed
@A-CGray A-CGray deleted the FixInterpolation branch July 19, 2024 17:04
@kanekosh kanekosh mentioned this pull request Jul 19, 2024
13 tasks
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.

4 participants