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

Fix CGNS mesh reader for multizone problems (one CGNS mesh per zone) #1566

Merged
merged 1 commit into from
Mar 24, 2022

Conversation

pcarruscag
Copy link
Member

Fixes #1564

@MicK7
Copy link
Contributor

MicK7 commented Mar 20, 2022

I don't think the fix is as simple as it seems.
Indeed looking back at how we developed the CGNS reader, It was originally meant to read multiple zone in a single file. But during development, someone decided to restrict the reader to only one zone per file (and I don't know if it was validated). So now we are seating in the middle.

If we replace line 169 of CGNSFVMMeshReader :

if ( nzones > 1 ) {
    SU2_MPI::Error(string("CGNS reader currently expects only 1 zone per CGNS file.") +
                   string("Multizone problems can be run with separate CGNS files for each zone."), CURRENT_FUNCTION);
}

by

 if ( cgnsZone > nzones) {
  cgnsZone = 1;
}

we can easily support multiple zone in one file.

To support one CGNS zone per file, I guess that user should provide either the index in the cgns file of the zone we want to read or even better its name and not rely on SU2 numbering of zones.

I think that supporting multiple mesh zones in the same file at the same time as one zone per mesh file should be possible as long as enough information is provided by the user.

In this case, I am wondering how the option MULTIZONE_MESH and MULTIZONE option are interacting in the related issue.

When MULTIZONE_MESH is set to NO do we expect one mesh file per zone ?
And in this case we can force CGNS Reader to read only the first Zone.

In a more generic way something like this should be possible:
MULTIZONE=YES
CONFIG_LIST= (zone_1.cfg, zone_2.cfg, zone_3.cfg)
CGNSZONENAMES = ("FluidRotor", "Solid", "FluidStator") # To let CGNS pick the right zone in the file and if it not found the first zone can be used (current SU2 behavior).

CGNSZONENAMES could also be set in each config file.

@pcarruscag
Copy link
Member Author

Thank you for having a look @MicK7.
Allowing multizone cgns based on the MULTIZONE_MESH option sounds reasonable but I did not want to remove the error without testing. Do you (or @ChristianBauerEng) have a multizone cgns we can use to test this?
I don't think we should add more config options.

@MicK7
Copy link
Contributor

MicK7 commented Mar 20, 2022

I can provide a multizone mesh.

Copy link
Contributor

@ChristianBauerEng ChristianBauerEng left a comment

Choose a reason for hiding this comment

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

Unfortunately I don't have a multizone-cgns mesh at the moment.
But I've tested this fix with my single-zone cgns meshes and I can confirm, that this fix works just as the documentation specifies.
MicK7's suggestion to make the reader also work with multi-zone cgns files would also be nice, but I understand that this requires a lot more work.

@pcarruscag
Copy link
Member Author

Thanks for testing 👍 it's only a bit more work, just needs a quick test.

1 similar comment
@pcarruscag
Copy link
Member Author

Thanks for testing 👍 it's only a bit more work, just needs a quick test.

@MicK7
Copy link
Contributor

MicK7 commented Mar 24, 2022

@pcarruscag I will create a separate PR for multizone cgns file.

@pcarruscag
Copy link
Member Author

Sounds good, thanks.

@pcarruscag pcarruscag merged commit cd4ecca into develop Mar 24, 2022
@pcarruscag pcarruscag deleted the fix_1564 branch March 24, 2022 10:30
@MicK7
Copy link
Contributor

MicK7 commented Mar 30, 2022

@pcarruscag I have a small file with two zones here: https://github.com/MicK7/TestcaseStage37
It is the classical NASA stage 37 (https://www.turbostream-cfd.com/case-studies/nasa-stage-37-stall-inception) but the file is 149 Mo. Do you think the size would not be too big to have it as a testcase ?
Sorry for replying on a closed PR ...

@pcarruscag
Copy link
Member Author

Great, thanks @MicK7. It is a bit big to have as a regression test just for this, but we can certainly use it to test the 1 or 2 lines we need to change to enable multizone cgns, are you planing to make those changes?

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

Successfully merging this pull request may close these issues.

3 participants