Renaming of PHYSICAL_PROBLEM config option to SOLVER by talbring · Pull Request #756 · su2code/SU2

talbring · 2019-08-09T16:13:36Z

Proposed Changes

While I was writing some documentation on how to set up a basic problem, I noticed that the PHYSICAL_PROBLEM had lost its meaning over the the last couple of years and it is hard to explain why it is called like that. It now refers actually to a numerical method that solves a particular problem and not to the physical problem. So I renamed it to SOLVER.

Additionally, I removed the REGIME_TYPE option and added the incompressible solvers as dedicated values (INC_EULER, INC_RANS, INC_NAVIER_STOKES) for the new SOLVER option. The method GetRegime_Type() is also removed and it is now explicitly checked for the kind of solvers. This way we remove the implicit assumption that every feature also works with the incompressible solvers. Developers have to deliberately add a check now ...

It is a lot of code copy at the moment, but once the restructuring is finished we should be able to remove a lot of it.

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.

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.

pcarruscag

The idea makes sense, but can you please keep the GetRegime method in CConfig (even if that is not used as an option anymore) and avoid changing its use for very long and inexpressive conditionals?
The "new" GetRegime method can implement the conditional or (preferably) return a value cached during setup.

pcarruscag · 2019-08-09T16:44:22Z

  /*--- Checks for incompressible flow problems. ---*/

-  if ((Kind_Solver == EULER) && (Kind_Regime == INCOMPRESSIBLE)) {
+  if (Kind_Solver == INC_EULER) {


This looks nice, more expressive, and will allow cleaner code to be written.

pcarruscag · 2019-08-09T16:47:16Z

-      else if (Ref_Inc_NonDim == INITIAL_VALUES) { cout << "Non-Dimensional simulation using intialization values." << endl; }
-      else if (Ref_Inc_NonDim == REFERENCE_VALUES) { cout << "Non-Dimensional simulation using user-specified reference values." << endl; }
-    }
+      if (Kind_Solver == EULER         || Kind_Solver == FEM_EULER         || Kind_Solver == DISC_ADJ_EULER ||


But this is the complete opposite, and it is all over the place so it is going to be a pain to maintain or even cleanup in the future.

talbring · 2019-08-12T13:41:03Z

@pcarruscag In general I agree with you. It is kind of messy. I reintroduced the Kind_Regime again. I don't want to deal with finding a better solution for that problem in this PR.

economon · 2019-08-13T06:25:51Z

I also like the idea of renaming to "SOLVER" but I would also say to avoid as much churn as possible in the conditionals throughout the code... looks like a wash when reading through the PR changes (almost as many +'s as -'s). Unless the changes are going to make something much more flexible or clear, I would say just keep Kind_Regime and set it in config postprocessing.

talbring · 2019-08-22T15:12:25Z

Anyone else has comments ?

pcarruscag · 2019-08-22T15:25:30Z

-  const bool compressible = (config->GetKind_Regime() == COMPRESSIBLE);
+  unsigned short Kind_Solver = config->GetKind_Solver();
+  const bool compressible = (Kind_Solver == FEM_EULER) ||
+                            (Kind_Solver == FEM_NAVIER_STOKES) ||


My comment is still valid I suppose.

Which one? I reverted most of the GetRegimeType calls. I deliberately left it in some places to make clear for what solvers this part of the code is intended for. For the future we have to find a better way of getting properties for a certain kind of solver.

pcarruscag · 2019-08-22T16:07:29Z

                   CURRENT_FUNCTION);

-  if(config->GetKind_Regime() != COMPRESSIBLE)
+  if(Kind_Solver != EULER && Kind_Solver != NAVIER_STOKES && Kind_Solver != RANS &&


This one is not solver specific

Yeah it is. It only works and is intended for the compressible flow solver. What happens if the kind solver is not a flow solver ? I still think that this way is much more obvious for which solvers this part of the code is intended (and tested) for.

Ok, we really need some more expressive and encapsulated way to express these things then.

pcarruscag · 2019-08-22T16:08:30Z

                   CURRENT_FUNCTION);

-  if(config->GetKind_Regime() != COMPRESSIBLE)
+  if(Kind_Solver != EULER && Kind_Solver != NAVIER_STOKES && Kind_Solver != RANS &&


None of these in MMS classes seem to be solver specific

pcarruscag · 2019-08-22T16:10:29Z


-  bool compressible   = (config->GetKind_Regime() == COMPRESSIBLE);
-  bool incompressible = (config->GetKind_Regime() == INCOMPRESSIBLE);
+  bool compressible   = false;


This you can keep the same without making the switch statement any more complicated.

Its even more complicated to understand when I keep it. Because then I'll have to add an if-statements later to distinguish between compressible/incompressible when actually allocating. This way you see immediately see what will be allocated (i.e. what boolean is set to true).

But anyway. All that this shows is that we have to do something about it at some point and neither approach adds or removes much readability. @rsanfer and myself have thought already about moving some the allocations to the iteration structure, as the only that class essentially has to know what is in the solver container. Likewise, the solver class is the only class that has to know what is in the numerics class and so on.

And ideally own the actual container they instantiate.

oleburghardt

I agree with the compromise you made here.

From looking through the files changed, we have a lot of additional lines in places where we are in fact just asking whether we are dealing with a flow solver. Doing that via GetKind_Solver() is not so elegant anyway. So they are really a non-issue if one expects that to be changed in the future.

I like that one can avoid the regime conditional this way. I already merged this branch in one of mine and it also improves at least visually the switch-kind-solver-sections.

talbring added 3 commits August 9, 2019 17:08

Renamed PHYSICAL_PROBLEM -> SOLVER and removed REGIME_TYPE

e3984f4

Adapted test case config

f37dece

Temp. changing tutorial branch

afe6521

pcarruscag requested changes Aug 9, 2019

View reviewed changes

talbring added 3 commits August 12, 2019 12:25

Reintroduced GetKind_Regime()

8979c77

Another round of fixes

f84b1de

Reverted some changes

c15fa77

talbring added 3 commits August 13, 2019 11:18

Small fixes

254db36

Merge branch 'develop' into feature_solvers

2f6a5ee

Reverted accidental change of reference file.

5b9e2ec

pcarruscag reviewed Aug 22, 2019

View reviewed changes

pr-triage Bot added the PR: unreviewed label Aug 22, 2019

pcarruscag reviewed Aug 22, 2019

View reviewed changes

Merge branch 'develop' into feature_solvers

8a2a9bd

oleburghardt approved these changes Aug 23, 2019

View reviewed changes

oleburghardt added PR: reviewed-changes-requested and removed PR: unreviewed labels Aug 23, 2019

pr-triage Bot added PR: unreviewed and removed PR: reviewed-changes-requested labels Aug 23, 2019

talbring mentioned this pull request Aug 23, 2019

SST turbulence model with sustaining terms #765

Merged

3 tasks

pcarruscag approved these changes Aug 23, 2019

View reviewed changes

pr-triage Bot added PR: reviewed-approved and removed PR: unreviewed labels Aug 23, 2019

pcarruscag merged commit f2fed54 into develop Aug 23, 2019

pcarruscag deleted the feature_solvers branch August 23, 2019 16:10

pr-triage Bot added PR: merged PR: reviewed-approved and removed PR: reviewed-approved PR: merged labels Aug 23, 2019

rsanfer mentioned this pull request Sep 2, 2019

Bug fix for vertex traction computation (new method) in incompressible flows. #785

Merged

4 tasks

talbring added the changelog:chore label Nov 7, 2019

talbring changed the title ~~Renaming PHYSICAL_PROBLEM to SOLVER~~ Renaming of PHYSICAL_PROBLEM config option to SOLVER Nov 8, 2019

pr-triage Bot added PR: merged and removed PR: reviewed-approved labels Nov 8, 2019

rsanfer mentioned this pull request Mar 3, 2020

Fix bug for incompressible FSI cases. #897

Merged

5 tasks

Uh oh!

Conversation

talbring commented Aug 9, 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!

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!

talbring commented Aug 12, 2019

Uh oh!

economon commented Aug 13, 2019

Uh oh!

talbring commented Aug 22, 2019

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 Aug 22, 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!

oleburghardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

talbring commented Aug 9, 2019 •

edited

Loading

pcarruscag Aug 22, 2019 •

edited

Loading