Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives #1428

jtneedels · 2021-11-08T01:27:18Z

Proposed Changes

Setting MUSCL reconstruction in CNEMOEulerSolver to use the minimum limiter val for all primitives, consistent with the feature_NEMO branch, to try and address issues with non-physical points using MUSCL with the NEMO solver in Develop.

Related Work

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, 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.

Signed-off-by: jtneedels <jneedels@stanford.edu>

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

Signed-off-by: jtneedels <jneedels@stanford.edu>

jtneedels · 2021-11-17T22:15:49Z

So instead of using arrays, since we are only choosing the smallest limiter value, it seems like we can just store a local variable in computing the Van Albada limiter. Does this look okay?. I have the feeling we want to generalize this for other limiters in the future, but this seems like the most efficien option for an implementation in CNEMOEulerSolver.

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

lgtm-com · 2021-11-17T23:04:17Z

This pull request introduces 1 alert when merging 9d134dd into a4836f4 - view on LGTM.com

new alerts:

1 for Empty branch of conditional

Signed-off-by: jtneedels <jneedels@stanford.edu>

jtneedels · 2021-11-18T01:42:46Z

I think this is working now, I'm seeing some non-physical reconstructed points initially for the axi_viscone test case, but these seem to go away after a few hundred iters using Van Albada. The solution looks physical. I'd like to change the regression tests to support these new values, but it would be great if everyone could double-check this before I make that change.

lgtm-com · 2021-11-18T01:43:31Z

This pull request introduces 1 alert when merging 8d3281c into a4836f4 - view on LGTM.com

new alerts:

1 for Empty branch of conditional

Signed-off-by: jtneedels <jneedels@stanford.edu>

pcarruscag

No need to generalize to other solvers IMO

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

pcarruscag · 2021-11-18T09:46:02Z

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

+  su2double        dTvedU_i[MAXNVAR] = {0.0},       dTvedU_j[MAXNVAR] = {0.0};
+  su2double           Eve_i[MAXNVAR] = {0.0},          Eve_j[MAXNVAR] = {0.0};


fyi these are not heap allocations, static arrays are on the stack and they do not cost anything to allocate.

Signed-off-by: jtneedels <jneedels@stanford.edu>

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>

WallyMaier

LGTM! Thanks for doing this!

Could you possibly post a before and after of a basic test case?

WallyMaier · 2021-11-18T18:32:16Z

SU2_CFD/src/output/CNEMOCompOutput.cpp

  /*--- Set the default convergence field --- */

-  if (convFields.empty() ) convFields.emplace_back("RMS_DENSITY");
+  if (convFields.empty() ) convFields.emplace_back("RMS_DENSITY_0");


jtneedels · 2021-11-20T21:53:27Z

LGTM! Thanks for doing this!

Could you possibly post a before and after of a basic test case?

before and after for the residuals? Or for the solution?

Here are the residuals for the serial regression of the axisymmetric viscous cone:

Signed-off-by: jtneedels <jneedels@stanford.edu>

TestCases/parallel_regression.py

jtneedels · 2021-11-23T21:11:29Z

Hi @pcarruscag , I notice that I get very slightly different residuals for the parallel regression running with 2 cores on my laptop vs in the Github regression test. @WallyMaier said this might be due to difference in partitioning, should I just use the residual values from the Github regression so the test passes, or are there other checks I should run?

pcarruscag · 2021-11-23T23:04:29Z

More likely to be due to compiler differences, yes please take the values from the github tests.

Signed-off-by: jtneedels <jneedels@stanford.edu>

…nto dev_nemo_muscl_fix

WallyMaier

LGTM!

change limiter to use min val, need to still fix minmod

996ee6a

Signed-off-by: jtneedels <jneedels@stanford.edu>

pr-triage bot added the PR: draft label Nov 8, 2021

jtneedels changed the base branch from master to develop November 8, 2021 01:27

pcarruscag reviewed Nov 8, 2021

View reviewed changes

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp Outdated Show resolved Hide resolved

WallyMaier changed the title ~~Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives~~ [WIP] Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives Nov 9, 2021

fixed lack of allocation for van albada

9d134dd

Signed-off-by: jtneedels <jneedels@stanford.edu>

WallyMaier reviewed Nov 17, 2021

View reviewed changes

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp Outdated Show resolved Hide resolved

jtneedels added 2 commits November 17, 2021 16:10

use built-in Van Albada limiter rather than formula

1fdce6f

Signed-off-by: jtneedels <jneedels@stanford.edu>

cleanup

8d3281c

Signed-off-by: jtneedels <jneedels@stanford.edu>

jtneedels requested review from CatarinaGarbacz, WallyMaier, fmpmorgado and pcarruscag November 18, 2021 01:42

remove empty conditional for LGTM warning

19a9fdc

Signed-off-by: jtneedels <jneedels@stanford.edu>

pcarruscag approved these changes Nov 18, 2021

View reviewed changes

jtneedels and others added 5 commits November 18, 2021 09:08

fix output

4d8e75b

Signed-off-by: jtneedels <jneedels@stanford.edu>

Update SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

b2ab2d0

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>

Update SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

7dd8f2c

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>

Update SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

7f04888

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>

Update SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

234b66b

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>

WallyMaier approved these changes Nov 18, 2021

View reviewed changes

jtneedels and others added 4 commits November 21, 2021 17:13

updating regression tests

88251b4

Signed-off-by: jtneedels <jneedels@stanford.edu>

Merge branch 'develop' into dev_nemo_muscl_fix

1cdb7b7

fixing test case?

4d961cb

Signed-off-by: jtneedels <jneedels@stanford.edu>

fixing test cases again

bee693e

Signed-off-by: jtneedels <jneedels@stanford.edu>

updating regression... again...

9569a49

Signed-off-by: jtneedels <jneedels@stanford.edu>

pcarruscag approved these changes Nov 22, 2021

View reviewed changes

TestCases/parallel_regression.py Outdated Show resolved Hide resolved

TestCases/parallel_regression.py Outdated Show resolved Hide resolved

TestCases/parallel_regression.py Show resolved Hide resolved

Fix merge

69e668d

updating parallel regression to match github vals

667308d

Signed-off-by: jtneedels <jneedels@stanford.edu>

jtneedels marked this pull request as ready for review November 24, 2021 00:08

pr-triage bot removed the PR: draft label Nov 24, 2021

Merge branch 'develop' into dev_nemo_muscl_fix

7f4bda7

jtneedels changed the title ~~[WIP] Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives~~ Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives Nov 24, 2021

pr-triage bot added the PR: unreviewed label Nov 24, 2021

jtneedels requested a review from WallyMaier November 24, 2021 02:17

jtneedels added 2 commits November 23, 2021 18:20

fixing small error in commenting in axi visc cone config file

d2389a0

Signed-off-by: jtneedels <jneedels@stanford.edu>

Merge branch 'dev_nemo_muscl_fix' of https://github.com/su2code/SU2 i…

2f69ee3

…nto dev_nemo_muscl_fix

WallyMaier approved these changes Nov 24, 2021

View reviewed changes

pr-triage bot added PR: partially-approved and removed PR: unreviewed labels Nov 24, 2021

jtneedels merged commit 60b91d7 into develop Nov 24, 2021

jtneedels deleted the dev_nemo_muscl_fix branch November 24, 2021 17:04

pr-triage bot added PR: merged and removed PR: partially-approved labels Nov 24, 2021

		su2double dTvedU_i[MAXNVAR] = {0.0}, dTvedU_j[MAXNVAR] = {0.0};
		su2double Eve_i[MAXNVAR] = {0.0}, Eve_j[MAXNVAR] = {0.0};

Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives #1428

Set MUSCL Reconstruction for NEMO in Develop to use smallest limiter val for all primitives #1428

Uh oh!

Conversation

jtneedels commented Nov 8, 2021

Proposed Changes

Related Work

PR Checklist

Uh oh!

Uh oh!

jtneedels commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Nov 17, 2021

Uh oh!

jtneedels commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Nov 18, 2021

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcarruscag Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

WallyMaier left a comment

Choose a reason for hiding this comment

Uh oh!

WallyMaier Nov 18, 2021

Choose a reason for hiding this comment

Uh oh!

jtneedels commented Nov 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtneedels commented Nov 23, 2021

Uh oh!

pcarruscag commented Nov 23, 2021

Uh oh!

WallyMaier left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtneedels commented Nov 17, 2021 •

edited

Loading

jtneedels commented Nov 18, 2021 •

edited

Loading

jtneedels commented Nov 20, 2021 •

edited

Loading