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

Overhauled node sorting in periodic geometry to be less buggy #500

Merged
merged 1 commit into from
Jan 25, 2018

Conversation

clarkpede
Copy link
Contributor

As described in Issue #431, using SU2_MSH for periodic mesh construction can be buggy due to its sorting of nodes. Under certain conditions, nodes can be duplicated and/or missing from the output *.su2 file.

This pull request implements a more robust sorting process, where the sorting occurs in the following steps:

  1. Send/receive nodes are explicitly identified
  2. The sorted list is created without any send/receive nodes.
  3. The receive and then send nodes are added to the sorted list.

Here's a few comments about the pull request:

  • Since this routine is only run once, I felt the usability of std::vector outweighed the computational efficiency of raw arrays.
  • I had to create two lists, NewSort and ReverseSort, instead of the original one list (NewSort). That's because two different mappings between node numbers need to occur. In the old code, where the list was sorted by switching elements, the two mappings should have been identical. Since the improved method does not use switching, two lists are necessary.
  • There's a lot of memory leaks/uninitialized values when checking this pull request with Valgrind. I checked them, and this pull request doesn't create any new memory issues. Apparently both SU2_CFD and SU2_MSH have memory issues.

Verification

Since none of the existing regression tests run SU2_MSH, I created a separate case to verify both the bug and the fix. It's a simple 3x3 cube that's periodic in the z-direction. I've attached the files below. Here's the steps:

  1. Run SU2_MSH MSH.cfg
  2. Run SU2_CFD per_CFD_dev.cfg

Using the existing develop branch, the verification case runs into a segfault when writing the output. This pull request allows the verification case to complete successfully.

You can also check the original test case from Issue #431 to verify the bug fix. If any of you know of additional verification tests I could run, then please let me know.

Test case: cube.tar.gz

This fixes #431

Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @clarkpede, and let’s keep #431 open to discuss next steps for periodic BC issues.

@economon economon merged commit 1871448 into su2code:develop Jan 25, 2018

ReverseSort.resize(NewSort.size());
for (iPoint = 0; iPoint < nPoint; iPoint++) {
unsigned short jPoint;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is defined as short unsigned.
It causes problems when the mesh has a higher number of nodes, changing it to long solves the issue in my case.

@LaSerpe
Copy link
Contributor

LaSerpe commented Feb 5, 2018

Hi guys,
I found a small flaw in the new code lines (line 18835). the counter should be defined as a long unsigned I guess.

@clarkpede
Copy link
Contributor Author

@LaSerpe thanks for catching that! Yeah, that definitely seems like a problem. I'll submit a patch.

@clarkpede clarkpede deleted the fix-periodic-MeshFile-sort branch May 11, 2018 16:50
CatarinaGarbacz pushed a commit that referenced this pull request Aug 26, 2019
Overhauled node sorting in periodic geometry to be less buggy
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.

3 participants