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

Adding sliding curves for DVGeoMulti #231

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

bernardopacini
Copy link
Contributor

Purpose

There are cases when using the intersection method in which a curve on a surface should be preserved to make sure that the patch does not change size or the curve is maintained to a certain shape. For example, maintaining the interface line between a propeller hub and nacelle is important to ensure a realistic boundary condition on each patch. This PR adds this sort of tracking.

These changes are actually made by @anilyil.

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

I have run this locally and also checked the derivatives to make sure the code is correct.

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

@bernardopacini bernardopacini added the enhancement New feature or request label Dec 2, 2023
@bernardopacini bernardopacini self-assigned this Dec 2, 2023
@bernardopacini bernardopacini requested a review from a team as a code owner December 2, 2023 21:50
Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e47a454) 65.04% compared to head (61ca21f) 65.18%.

Files Patch % Lines
pygeo/parameterization/DVGeoMulti.py 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   65.04%   65.18%   +0.13%     
==========================================
  Files          47       47              
  Lines       12112    12123      +11     
==========================================
+ Hits         7878     7902      +24     
+ Misses       4234     4221      -13     

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

@bernardopacini
Copy link
Contributor Author

I added a test for the sliding curve feature as discussed offline with @hajdik and @sseraj. It checks to ensure that a few points remain on the lines on which they were defined, but allowed to vary along them (@anilyil let me know if it is okay with you too).

Copy link
Contributor

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

This is a great feature. Thanks for adding the test as well. I have a few minor comments

tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
tests/reg_tests/test_DVGeometryMulti.py Outdated Show resolved Hide resolved
pygeo/parameterization/DVGeoMulti.py Show resolved Hide resolved
pygeo/parameterization/DVGeoMulti.py Outdated Show resolved Hide resolved
pygeo/parameterization/DVGeoMulti.py Outdated Show resolved Hide resolved
@bernardopacini
Copy link
Contributor Author

Thanks for the feedback @sseraj! I updated the files with your comments and defer to @anilyil on the implementation-specific comments.

@anilyil
Copy link
Contributor

anilyil commented Dec 23, 2023

@sseraj, I addressed both of your comments. Also, thank you for spelling out pretty much all of the changes needed in the comments. Both things you said are correct. Please review my changes and mark the conversations as resolved.

@bernardopacini, this PR is good for another round of reviews, correct?

@bernardopacini
Copy link
Contributor Author

@sseraj, I addressed both of your comments. Also, thank you for spelling out pretty much all of the changes needed in the comments. Both things you said are correct. Please review my changes and mark the conversations as resolved.

@bernardopacini, this PR is good for another round of reviews, correct?

I just updated the assertions as recommended by @sseraj, now should be good to go.

@sseraj sseraj merged commit 9bbcdf0 into mdolab:main Jan 17, 2024
12 checks passed
@bernardopacini bernardopacini deleted the DVGeoMulti-SlidingCurves branch January 17, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants