Skip to content

Conversation

@maxaehle
Copy link
Contributor

Proposed Changes

Remove ununsed multigrid options in cfg file of the new NACA 0012 fixed_values testcase and the SST_SUST testcase, and some other small changes.

Related Work

Continuation of #1236, comments by @pcarruscag and @TobiKattmann

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.

  • [X ] 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.

added screen output for LINSOL_RESIDUAL and RMS_DENSITY,
and removed multigrid stuff
@maxaehle
Copy link
Contributor Author

We could also remove the unused *_ADJ_* options from the cfg file

Parallel and hybrid regression tests for NACA0012 SST/SST_SUST,
as well as the Python wrapper tests, now check rms[Rho] and
LinSolRes as well.
turb_naca0012_sst_fixedvalues added to parallel and hybrid
regression tests.
@maxaehle
Copy link
Contributor Author

Commit 1bc9649 reverts my changes to those residuals in hybrid_regression.py which were covered by the regression tests already before. (Oddly enough, on my system the residuals of SU2_CFD -t 2 were different from those computed by the Github pipeline. Maybe this is due to floating-point discrepancies when arithmetic operations are rearranged?)

@pcarruscag
Copy link
Member

That is possible the hybrid regression uses single precision for linear solvers.

@TobiKattmann
Copy link
Contributor

👍 for doing this @maxaehle .

  • I am in favor of removing the _ADJ_ options with the GRAD_OBJFUNC_FILENAME.
  • As Roe is used the JST_SENSOR_COEFF can go. Limiter are turned off so either the slope limiter section with 2 options can be removed or be moved down below the FLOW NUMERICAL METHOD DEFNITION.
  • CONV_CRITERIA will be gone once you merge develop and you can add CONV_FIELD=RMS_DENSITY as that is also the default. Removign the _CAUCHY_ options is prob debatable, but in this very cfg they are unused.
  • The CFL_ADAPT* stuff can go (unless the values are chosen in such a way by you that they do work well, otherwise it might even do negative advertisement for an option)

I guess I am also a bit aggressive on stripping options from files, so take it with a grain of salt 🧂 .

Another aspect ,which is of course nothing to be changed in this PR and is also a bit off-topic, is that lists like SOLVER change over time but the list in the comment over the option in the Testcase cfg does not get updated (there might of course be good examples of people doing so ;) ). WAVE_EQUATION and POISSON_EQUATION e.g. are not available anymore, yet they are in the cfg description of SOLVER here... and 200 other Testcases. So a way might be to strip those and sth like

% See the config_template.cfg in the code root for more information on the options

... although the info there are not always perfect either, but one place to maintain is easier than a couple hundred

@TobiKattmann
Copy link
Contributor

Hey @maxaehle , if you don't want to make further changes you can hit the merge if you like, once the tests pass.

@maxaehle maxaehle merged commit 533a2c4 into develop Mar 31, 2021
@maxaehle maxaehle deleted the fix_turb_fixed_values branch March 31, 2021 09:36
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.

4 participants