Solution output in CGNS format by baldang · Pull Request #1387 · su2code/SU2

baldang · 2021-10-01T20:21:05Z

Proposed Changes

Added CGNS output format. Master node writes the output files and receives data from other processes (one at a time for each field to avoid memory limit problems). Some suggestions from feature_CGNS_output are included in the code. However, this first version produce a new file, mesh and solution, at each call as in the Paraview format. The code support CGNS and SURFACE_CGNS in the OUTPUT_FILES option

PR Checklist

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
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.

lgtm-com · 2021-10-01T21:13:28Z

This pull request introduces 2 alerts when merging 963e7c7 into 58c580b - view on LGTM.com

new alerts:

2 for Multiplication result converted to larger type

pcarruscag

Nice 👍 The code looks generally ok, but there is room to simplify.

SU2_CFD/include/output/filewriter/CCGNSFileWriter.hpp

SU2_CFD/src/output/filewriter/CCGNSFileWriter.cpp

SU2_CFD/src/output/COutput.cpp

baldang · 2021-10-02T15:49:21Z

Thank you for all the suggestions. Now, they should be all included in the code. Furthermore, I added the check when SU2 is not built with CGNS support as done in the Tecplot case.

pcarruscag

Thank you for the changes.
More suggestions below, you should be able to apply them directly in GitHub, also see the questions below and please add const to all the variables where that is possible.

SU2_CFD/include/output/filewriter/CCGNSFileWriter.hpp

SU2_CFD/src/output/filewriter/CCGNSFileWriter.cpp

lgtm-com · 2021-10-02T16:36:09Z

This pull request introduces 1 alert when merging 74ce20b into 58c580b - view on LGTM.com

new alerts:

1 for Multiplication result converted to larger type

lgtm-com · 2021-10-02T18:20:09Z

This pull request introduces 1 alert when merging 9ec9fe2 into 58c580b - view on LGTM.com

new alerts:

1 for Multiplication result converted to larger type

pcarruscag

Welcome to SU2 and thank you for the contribution! Give it some more time (~1 week) for people to review and test your contribution and then merge.

SU2_CFD/src/output/filewriter/CCGNSFileWriter.cpp

Co-authored-by: Pedro Gomes <[email protected]>

SU2_CFD/include/output/filewriter/CCGNSFileWriter.hpp

lgtm-com · 2021-10-02T20:49:18Z

This pull request introduces 1 alert when merging 8b0ae98 into 58c580b - view on LGTM.com

new alerts:

1 for Multiplication result converted to larger type

WallyMaier

LGTM!

WallyMaier · 2021-10-02T22:36:35Z

Common/src/CConfig.cpp

  }
 #endif

+  /*--- Check if SU2 was build with CGNS support, as that is required for CGNS output. ---*/


pcarruscag · 2021-10-08T09:37:42Z

Hi @baldang, it seems we have some problems with Windows builds -> https://github.com/su2code/SU2/runs/3836680912?check_suite_focus=true
Can you have a look please? Seems to be related with the CGNS int types, maybe we need to typedef these in a special way for compatibility with Windows.
However we also use those types in the CGNS mesh reader... maybe @MicK7 can give some quick pointer to the solution.

baldang · 2021-10-08T09:48:34Z

Hi @pcarruscag, it sounds strange this error since before opening the PR I ran the same tests on my repository and it compliled correctly. Here you can have a look at it https://github.com/baldang/SU2/runs/3778439078?check_suite_focus=true . I will let you know if I find a solution.

pcarruscag · 2021-10-08T09:57:53Z

Hmm ok I'm re-running the tests, maybe something went wrong because I merged without merging develop first.

baldang · 2021-10-08T10:08:16Z

The win build failed again but I have just ran the same tests on my forked repository and it builds correctly.

baldang · 2021-10-11T20:53:28Z

Hi @pcarruscag, I went trough the error in the Windows build and it is related to cgsize_t type definition contained in the CGNS library and not in the implemented code. It sounds strange since that part of code was already present in the code and cgsize_t was already used in CCGNSMeshReaderFVM class. To be sure, I removed the forked SU2 from my repositories and then I forked it again to have an exact copy of develop branch. I ran "Relase Management" pipeline and everything is fine. Do you have any suggestion regarding this issue?

pcarruscag · 2021-10-12T15:45:57Z

Hi @baldang, problem solved: bba499e
I kinda found the solution here https://sourceforge.net/p/mingw-w64/mailman/message/24692038/
I don't know why this built in your fork.

baldang added 2 commits September 30, 2021 21:58

added output in CGNS format

e4b0744

added test case and documentation

963e7c7

pr-triage bot added the PR: unreviewed label Oct 1, 2021

pcarruscag reviewed Oct 1, 2021

View reviewed changes

add CGNS import check and clean up code

74ce20b

pcarruscag reviewed Oct 2, 2021

View reviewed changes

removed HAVE_MPI and minor fix

9ec9fe2

Merge branch 'develop' into feature_outputCGNS

b35b607

pcarruscag approved these changes Oct 2, 2021

View reviewed changes

SU2_CFD/src/output/filewriter/CCGNSFileWriter.cpp Outdated Show resolved Hide resolved

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Oct 2, 2021

const "isSurface"

81346a3

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Oct 2, 2021

minor fix

5b18c96

Co-authored-by: Pedro Gomes <[email protected]>

pcarruscag reviewed Oct 2, 2021

View reviewed changes

SU2_CFD/include/output/filewriter/CCGNSFileWriter.hpp Show resolved Hide resolved

private auxilary member functions

8b0ae98

pcarruscag added the changelog:feature label Oct 2, 2021

pcarruscag changed the title ~~feature_outputCGNS~~ Solution output in CGNS format Oct 2, 2021

baldang added 2 commits October 2, 2021 23:07

fix meson

54634d2

minor fix

7b81d08

WallyMaier reviewed Oct 2, 2021

View reviewed changes

Merge branch 'develop' into feature_outputCGNS

af0c29e

pcarruscag merged commit f12cd07 into su2code:develop Oct 8, 2021

pr-triage bot removed the PR: unreviewed label Oct 8, 2021

pr-triage bot added the PR: merged label Oct 8, 2021

Conversation

baldang commented Oct 1, 2021

Proposed Changes

PR Checklist

Uh oh!

lgtm-com bot commented Oct 1, 2021

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baldang commented Oct 2, 2021

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Oct 2, 2021

Uh oh!

lgtm-com bot commented Oct 2, 2021

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Oct 2, 2021

Uh oh!

WallyMaier left a comment

Choose a reason for hiding this comment

Uh oh!

WallyMaier Oct 2, 2021

Choose a reason for hiding this comment

Uh oh!

pcarruscag commented Oct 8, 2021

Uh oh!

baldang commented Oct 8, 2021

Uh oh!

pcarruscag commented Oct 8, 2021

Uh oh!

baldang commented Oct 8, 2021

Uh oh!

baldang commented Oct 11, 2021

Uh oh!

pcarruscag commented Oct 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants