Skip to content

Fix logic bugs in 2D Actuator Disk preprocessing and Periodic Solver #2694

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.

Sign up for GitHub

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

Jump to bottom
Open
shbhmexe wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Fix logic bugs in 2D Actuator Disk preprocessing and Periodic Solver #2694

shbhmexe wants to merge 7 commits into from

Conversation

@shbhmexe
Copy link
Contributor

@shbhmexe shbhmexe commented Jan 11, 2026 •
edited
Loading

This PR fixes logic bugs in the 2D Actuator Disk surface splitting code in CSU2ASCIIMeshReaderFVM.cpp:

  1. Fix wrong loop variable: Changed ActDiskPoint_Front[iEdge] to ActDiskPoint_Front[iPoint] - was using wrong variable causing incorrect perimeter detection
  2. Fix wrong array access: Changed EdgeBegin[...] to ActDiskPoint_Front[...] - was indexing wrong array causing potential segfault
  3. Remove incorrect unique() call: The perimeter detection needs frequency information which unique() was destroying

Related Work

No related PRs or issues.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

@shbhmexe
Fix in 2D Actuator Disk splitting and Periodic LS Solver
5484448 
- CSU2ASCIIMeshReaderFVM.cpp: Fix uniqueness logic and array indexing in 2D Actuator Disk splitting.
- CSolver.cpp: Fix matrix corruption in Periodic Least Squares (replace duplicate r23_b with 0.0).

Signed-off-by: shbhmexe <[email protected]>
@shbhmexe shbhmexe changed the base branch from master to develop January 11, 2026 09:59
@shbhmexe
Fix bugs in 2D Actuator Disk and Periodic Solver
59d0825 
- CSU2ASCIIMeshReaderFVM.cpp: Fix uniqueness logic and array indexing in 2D Actuator Disk splitting.

- CSolver.cpp: Fix matrix corruption in Periodic Least Squares (replace duplicate r23_b with 0.0).

Signed-off-by: shbhmexe <[email protected]>
@shbhmexe
Fix style: split multiple statements to separate lines in CSolver.cpp
ff332fe
shbhmexe and others added 4 commits January 12, 2026 13:58
@shbhmexe
Remove unused variable r23_b to fix build error
15b77ed
@shbhmexe
Delete PR_REPORT.md
57f595e
@bigfooted
Merge branch 'develop' into fix/2d-actuator-disk-and-periodic-solver-…
5d90b26 
…bugs
@shbhmexe
Revert CSolver.cpp changes, keep only CSU2ASCIIMeshReaderFVM.cpp fixes
d350a8a 
- Restore CSolver.cpp to original state (r23_b changes were incorrect)

- Keep CSU2ASCIIMeshReaderFVM.cpp fixes: unique() removal and iEdge->iPoint fix

Signed-off-by: shbhmexe <[email protected]>
pcarruscag
pcarruscag reviewed Jan 12, 2026
View reviewed changes
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

Do you have before/after results?

@shbhmexe
Copy link
Contributor Author

shbhmexe commented Jan 14, 2026

Do you have before/after results?

No before/after numerical results available as this fixes 2D Actuator Disk code path which has no regression test coverage.

The changes are logical corrections:

Fix wrong variable iEdge → iPoint in loop
Fix wrong array EdgeBegin → ActDiskPoint_Front
Remove unique() that was destroying frequency info needed for perimeter detection
These would cause incorrect mesh splitting or segfaults for 2D actuator disk cases.

@shbhmexe
Copy link
Contributor Author

shbhmexe commented Jan 14, 2026

and also check this pr #2669 and #2663

@pcarruscag
Copy link
Member

pcarruscag commented Jan 14, 2026

You'll need to add a regression test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcarruscag pcarruscag pcarruscag left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

changelog:fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shbhmexe @pcarruscag @bigfooted