All new output framework for customizable screen, history and volume output #724

talbring · 2019-06-28T09:56:15Z

Proposed Changes

This PR introduces a completely rewritten output structure. If you have ever looked into the old output_structure.cpp file, you probably noticed that it was quite messy. To add new output to screen or history people just copied what was there and modified it to suit there own needs. This resulted in a lot of if/switch statements and was hard to read ... one reason for this was that we have not really used an object oriented structure for the output.

Despite rearranging everything into separate classes, here is a non-exhaustive list of new features and changes:

Fully customizable screen, history and volume output
New multizone screen output
New flexible way so set convergence criteria (any residual and/or coefficient can be used as a monitor)
Consistent naming of iteration variables (TIME_ITER, OUTER_ITER, INNER_ITER)
Consistent file naming options (RESTART_FILENAME, SOLUTION_FILENAME)
On-the-fly writing of all visualization files
New Singlezone and Multizone drivers are now used by default.

Note that this is a very big change. And i apologize for any inconvenience and troubles that this might cause. I can only maintain features covered by the regression tests. So please test it with your cases and let me know if anything is missing (especially in the output).

I will create documentation for all the new features soon. If you want to test it already, please let me know, I need some feedback.

User Documentation on the changes:
https://su2code.github.io/docs/Solver-Setup
https://su2code.github.io/docs/Custom-Output
https://su2code.github.io/docs/Guide-to-v7/

Related Work

#715 is already included in this PR and should be merged before.

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

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).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.

SU2_CFD/src/output/filewriter/CFVMDataSorter.cpp

SU2_CFD/src/output/filewriter/CSU2BinaryFileWriter.cpp

economon · 2019-07-08T16:04:53Z

SU2_CFD/src/output/COutput.cpp

+
+  /// BEGIN_GROUP: ITERATION, DESCRIPTION: Iteration identifier.
+  /// DESCRIPTION: The time iteration index.
+  AddHistoryOutput("TIME_ITER",     "Time_Iter",  FORMAT_INTEGER, "ITER"); 


I had a suggestion to improve the config option tracking/documentation (and keep it automatically up to date), and I think we could try it here too for the new output options that are registered.

Is it easy for you to add the string description as an input directly into the prototype for AddHistoryOutput() as well as add a print_description() method that would write it to the console (or write all registered options in one loop)? That way, if a user wanted to get the list of all available outputs for a particular solver, we could have a "help" option in the config file / at the command line that could first print all of the group names with their descriptions, followed by all available fields within a chosen group with their string descriptions. Should be easier to maintain and expose, since the descriptions will be required directly in the C++ whenever adding the option.

We could consider adding a similar self-describing approach for all config options if this works.

pcarruscag

This really looks much cleaner @talbring !
I am a bit worried by the efficiency of the implementation however, it seems to me the maps (see comments below) are being accessed too often for little reason.

pcarruscag · 2019-07-11T12:52:44Z

Common/include/toolboxes/printing_toolbox.hpp

+}
+
+inline int stoi(const std::string s){
+  std::istringstream ss(s);


These 3 methods could be templates

pcarruscag · 2019-07-11T12:56:55Z

Common/include/toolboxes/signal_processing_toolbox.hpp

+    }
+
+    su2double Get(){
+      return val;


I think for new code we should try to declare methods const when possible.

pcarruscag · 2019-07-11T13:01:10Z

Common/src/meson.build

@@ -0,0 +1,66 @@
+common_include = include_directories('./')
+common_src =files(['geometry_structure_fem_part.cpp',
+           'primal_grid_structure.cpp',


This system does look quite clean.

pcarruscag · 2019-07-11T13:05:26Z

Common/src/toolboxes/signal_processing_toolbox.cpp

+  for (std::vector<su2double>::iterator it = data.begin(); it != data.end(); it++){
+    avg += (*it);
+  }
+  return avg/=data.size();


sure you want /= and not just /?

pcarruscag · 2019-07-11T13:11:04Z

SU2_CFD/include/output/COutput.hpp

+  };
+
+  std::map<string, HistoryOutputField >         HistoryOutput_Map;    /*!< \brief Associative map to access data stored in the history output fields by a string identifier. */
+  std::vector<string>                           HistoryOutput_List;   /*!< \brief Vector that contains the keys of the HistoryOutput_Map in the order of their insertion. */


Does Doxygen require the comment to be in front of the variable? (These lines are very long).

pcarruscag · 2019-07-11T13:48:29Z

SU2_CFD/include/output/COutput.hpp

+   * \param[in] groupname - The name of the group this field belongs to.
+   */
+  inline void AddVolumeOutput(string name, string field_name, string groupname){
+    VolumeOutput_Map[name] = VolumeOutputField(field_name, -1, groupname);


Why are so many of these strings passed by value instead of const& ?

pcarruscag · 2019-07-11T14:05:13Z

SU2_CFD/src/output/COutput.cpp

+   * before writing it to file ---*/
+
+  Local_Data.resize(nPoint, std::vector<su2double>(GlobalField_Counter, 0.0));
+


This is not a very good way to store the data, have you considered flattening this into a single dimension or at least transposing it?

After looking at the parallelization part I understand why it cannot be transposed.
I am going to write a re-usable 2D container for CVariable, we could eventually use that.

pcarruscag · 2019-07-11T14:15:32Z

SU2_CFD/src/output/CAdjElasticityOutput.cpp

+  CPoint*    Node_Geo  = geometry->node[iPoint];
+
+  SetVolumeOutputValue("COORD-X", iPoint,  Node_Geo->GetCoord(0));
+  SetVolumeOutputValue("COORD-Y", iPoint,  Node_Geo->GetCoord(1));


You are hashing into the map a bunch of times per node but the access sequence is always the same, you could cache the offset (column?) during preprocessing and not go through the map here.

Thanks for mentioning this! That was indeed not very efficient. I added caching of the offset. That speeds up the whole process a lot. I implemented it in a way so that the order the SetVolumeOutputValue calls can be still different from the order of the AddVolumeOutput calls.

Looks good @talbring, should do the trick provided the function is not called twice for the same variable.
Will the list of output variables be exposed to the config? (I remember from yours and @rsanfer 's presentation that the screen output will be) Is that why the variables are being referenced via strings? Have you considered a pair of enum/map like we have for other options? That way you could create a vector of enum instead of a vector of strings and use that as a "map" which would be efficient without needing to rely on access order, etc.

What do you mean exactly by "exposing it to the config" ? The rational behind having it as strings is that it is independent of the config structure. So just adding output doesn't require to recompile the whole code.

I added a further check to make sure that the SetVolumeOutputValue() was not called twice for the same field. The good thing is that by adding these checks I already found some bugs :)

I mean having a config option that allows the user to pick which fields they want in the output file.
If that is not the objective and everything is known at compile time, maps are an over the top solution IMO.

Oh thats what you mean! You can specify it in the config file. That was also an important point of the implementation.

There are the options

VOLUME_OUTPUT SCREEN_OUTPUT HISTORY_OUTPUT

to specify the fields you want to have. The config structure just has a string list for these options and no enum/map combination.

A more detailed description follows soon.

Ok I'll hold further comments until then, but if the concern with the enum/map pair is compilation dependencies those can be defined at the level of the COutput children classes.

pcarruscag · 2019-07-11T14:22:17Z

SU2_CFD/src/output/filewriter/CFVMDataSorter.cpp

+  }
+
+  /*--- Launch the non-blocking sends of the connectivity. ---*/
+


Is this comment correct? Is it not data you are sending?

pcarruscag · 2019-07-11T14:25:21Z

SU2_CFD/include/output/filewriter/CFEMDataSorter.hpp

+class CFEMDataSorter : public CParallelDataSorter{
+private:
+
+  std::vector<std::vector<su2double> >* Local_Data;


Please make this const* or const&

talbring · 2019-10-02T13:38:54Z

From my side everything is ready for now.

pcarruscag · 2019-10-02T14:45:16Z

@talbring you seem to have a few trailing blanks in the new files, after the pending PR's are merged can you please clean them up.

jayantmukho · 2019-10-02T15:20:05Z

The .travis.yml file is currently cloning the feature_input_output branch of the TestCases and the Tutorials folders. Should those branches be merged with develop in their respective repos? And consequently, should the .travis.yml file be changed to point to the develop branches?

I ask because I am going to have to do something similar for #780 and I am not sure what the process for doing that would be.

pcarruscag · 2019-10-02T15:31:29Z

That is the process I followed so far Jayant Mukhopadhaya <notifications@github.com> escreveu no dia quarta, 2/10/2019 à(s) 16:20:

…

The .travis.yml file is currently cloning the feature_input_output branch of the TestCases and the Tutorials folders. Should those branches be merged with develop in their respective repos? And consequently, should the .travis.yml file be changed to point to the develop branches? I ask because I am going to have to do something similar for #780 <#780> and I am not sure what the process for doing that would be. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#724?email_source=notifications&email_token=AJCOXN42AZJMAB5I3M7S5GDQMS33HA5CNFSM4H4D5RW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAFEF6I#issuecomment-537543417>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJCOXN2S57FMOMAZNXV2ERTQMS33HANCNFSM4H4D5RWQ> .

pcarruscag · 2019-10-04T10:35:05Z

TestCases/parallel_regression_AD.py

-    discadj_fsi.timeout   = 1600
-    discadj_fsi.tol       = 0.00001
-    test_list.append(discadj_fsi)
+#    # Structural model


@talbring I'll start by saying I hope this was an accident, which is possible given the size of the pull request...

I found out yesterday the FSI discrete adjoint capabilities of the code no longer work, needless to say I am not very happy. I am sure this work could have been done without disrupting the functionalities of the code for people that actively use it, develop it, maintain it, and actively interact with the community.
I could have helped keeping this working while an alternative is not available, I clearly showed interest and availability to help, so, if this was not done accidentally, it is unfair to say the least.

There is no need to revert anything, but for what is worth I will disapprove any release related pull requests until the functionally is put back in the code. I will try to help on #774 put I would have preferred to do so without having to rush.

talbring · 2019-10-04T11:37:11Z

@pcarruscag Yes, completely forgot about this. I can probably bring the functionality back in without using or waiting for #774 to make the regression test pass. I'll talk to @rsanfer next week to split the config files into the new multi-zone format.

economon reviewed Jul 8, 2019

View reviewed changes

talbring added 2 commits July 11, 2019 10:52

Fix for AD

f7244fc

Adapted more test cases

abfe70a

pcarruscag reviewed Jul 11, 2019

View reviewed changes

talbring added 26 commits July 11, 2019 17:49

Added caching of offset for setting output values

8bfb5c1

Small fix

9d30aa3

Merge remote-tracking branch 'origin/develop' into feature_input_output

d893ecc

Small fix

f20b609

cleaned up includes and added forward decl. where possible

72f5b72

Added missing include

88da937

Merge remote-tracking branch 'origin/develop' into feature_input_output

bf00947

Removing merging leftover

25c5d4f

Removed unused routine

f9bac10

Added one more check to the Offset_Cache

f01e158

Fixed some test cases

4966624

Merge remote-tracking branch 'origin/develop' into meson_build

b8939b7

Added files to meson

c0ca542

Readded MARKER_FLUID_INTERFACE option to test cases

f3c7a5d

Adapted unsteadyCHT pywrapper case

5a69fc7

Removed old InitSlidingState routine

d984aac

Merge remote-tracking branch 'origin/develop' into feature_input_output

c08c421

Fixing test cases

c18c962

Changed offset cache implementation

41fdf15

Updated some test cases

f1167dd

Fixed reading of surface_adjoint.csv for SU2_DOT

af0598e

Further change to offset cache ..

8d729fe

Further changes to test cases

260e769

Merge remote-tracking branch 'origin/develop' into feature_input_output

5878218

Added files to meson

02db0eb

Added total flow coeff values

b197aae

talbring added 8 commits October 1, 2019 17:11

Changed aero coeff names to be consistent with python scripts

081e6a5

Added a new history mapping procedure for python scripts

39dbebc

Added consistency check for direct diff python script

96b8d5f

Updated fixed cl test values

9937172

Fixing order of dict

ab1053c

Adapting test case values after merge with develop

e67de4a

Fixed unused import

e52d2a3

Further update to test values

e5d7245

pcarruscag merged commit a654447 into develop Oct 2, 2019

pr-triage bot added PR: merged and removed PR: unreviewed labels Oct 2, 2019

economon mentioned this pull request Oct 2, 2019

New Nonlinear Iteration Controller for Improved Fluid Solver Robustness & Convergence #790

Merged

4 tasks

pcarruscag reviewed Oct 4, 2019

View reviewed changes

pr-triage bot added PR: unreviewed and removed PR: merged labels Oct 4, 2019

economon mentioned this pull request Oct 7, 2019

Restructuring of CVariable into a Contiguous Data Structure #753

Merged

4 tasks

rsanfer mentioned this pull request Oct 15, 2019

Re-enable Discrete Adjoint FSI Test Cases #803

Merged

4 tasks

talbring deleted the feature_input_output branch October 30, 2019 17:18

WallyMaier mentioned this pull request Oct 30, 2019

Replace the static char buffers at the beginning of the SetConvergence_History() routine in output_structure.cpp #66

Closed

talbring added the changelog:feature label Nov 6, 2019

talbring changed the title ~~New Output Structure~~ All new output framework for customizable screen, history and volume output Nov 8, 2019

pr-triage bot added PR: merged and removed PR: unreviewed labels Nov 8, 2019

snow54 mentioned this pull request Dec 16, 2019

SU2 v7 NOT writing a nearfield_flow.dat and Equivalent_Area.dat for Sonicboom case #841

Closed

snow54 mentioned this pull request Jul 19, 2021

Fix equivalent area calculation #1329

Merged

5 tasks

		* before writing it to file ---*/

		Local_Data.resize(nPoint, std::vector<su2double>(GlobalField_Counter, 0.0));

		}

		/--- Launch the non-blocking sends of the connectivity. ---/

All new output framework for customizable screen, history and volume output #724

All new output framework for customizable screen, history and volume output #724

Uh oh!

Conversation

talbring commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcarruscag Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talbring commented Oct 2, 2019

Uh oh!

pcarruscag commented Oct 2, 2019

Uh oh!

jayantmukho commented Oct 2, 2019

Uh oh!

pcarruscag commented Oct 2, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talbring commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

talbring commented Jun 28, 2019 •

edited

Loading

pcarruscag Jul 15, 2019 •

edited

Loading