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

Zipper mesh segfault fix #193

Merged
merged 3 commits into from
Feb 24, 2022
Merged

Zipper mesh segfault fix #193

merged 3 commits into from
Feb 24, 2022

Conversation

sseraj
Copy link
Collaborator

@sseraj sseraj commented Feb 23, 2022

Purpose

This fixes the zipper mesh segfault issue first reported by @awccopp.

When creating the zipper mesh, indexing out of bounds can occur when the number of closest points we are searching for is greater than the total number of points on a string. Capping the initial nSearch in the stringMatch routine solves the issue.

The current initial value for nSearch is 50, which explains why the problem only appears on small meshes like @awccopp's airfoil cases. I added a test with a simple overset cube mesh that can reproduce the error.

Expected time until merged

Regular urgency; one week

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

Run test_zipper.py without compiling the fix. The result will be a segfault (or 'index outside of expected range' if compiled with a bounds check flag).
Run the test again after recompiling. The test should now pass.

Checklist

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • 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

@sseraj sseraj requested a review from awccopp February 23, 2022 16:41
@sseraj sseraj requested a review from a team as a code owner February 23, 2022 16:41
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #193 (b11d794) into master (9266f4d) will decrease coverage by 12.55%.
The diff coverage is n/a.

❗ Current head b11d794 differs from pull request most recent head 9ae5eee. Consider uploading reports for the commit 9ae5eee to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #193       +/-   ##
===========================================
- Coverage   43.27%   30.72%   -12.56%     
===========================================
  Files          15       15               
  Lines        3561     3561               
===========================================
- Hits         1541     1094      -447     
- Misses       2020     2467      +447     
Impacted Files Coverage Δ
adflow/pyADflow.py 46.98% <0.00%> (-20.27%) ⬇️
adflow/MExt.py 78.94% <0.00%> (-18.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9266f4d...9ae5eee. Read the comment docs.

@joanibal
Copy link
Collaborator

Nice work Sabet!

@anilyil anilyil requested a review from joanibal February 24, 2022 02:13
@anilyil
Copy link
Contributor

anilyil commented Feb 24, 2022

it looks like we need another approval? I tried requesting one from @joanibal since he already approved but github didnt like that

Copy link
Contributor

@awccopp awccopp left a comment

Choose a reason for hiding this comment

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

I pulled your fixes and ran them locally on some cases where I had this issue and they all ran fine. Looks good to me thanks for fixing.

@sseraj sseraj merged commit ba6f8a9 into mdolab:master Feb 24, 2022
@sseraj sseraj deleted the zipper-index branch February 24, 2022 14:30
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