[WIP] Fixes to axi nemo set functions and auxvargrad in source residual #1205

jtneedels · 2021-02-24T02:11:57Z

fixed set functions for NEMO axisymmetric, implemented auxvargrad correction in nemo axisymmetric source residual, fixed fluid model/transport model print statement

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

Proposed Changes

This PR addresses an issue found with not setting primitive and conservative variables in computing the axisymmetric source residual, as well as implementing the auxvargrad indexing correction applied in the standard air axisymmetric source residual and fixing an issue with printing the fluid and transport models.

Related Work

This PR is related to #1192 and #1162.

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.

…rection in nemo axisymmetric source residual, fixed fluid model/transport model print statement Signed-off-by: jtneedels <jneedels@stanford.edu>

WallyMaier

Thanks for fixing these issues, @jtneedels. Ill let someone else check, but LGTM.

WallyMaier · 2021-02-24T04:31:26Z

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

+          // Do not remove.
+
+          /*--- Set volume ---*/
+          numerics->SetVolume(geometry->nodes->GetVolume(iPoint));


I think this is fine. We might need to look into a way to avoid repeating "set" functions in the future.

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

WallyMaier · 2021-02-24T05:06:47Z

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

    ModelTable.AddColumn("Mixture", 25);
-    ModelTable.AddColumn("Transport Model", 25);
    ModelTable.AddColumn("Fluid Model", 25);
+    ModelTable.AddColumn("Transport Model", 25);


Thanks for aligning these!

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

pcarruscag · 2021-02-24T09:44:31Z

SU2_CFD/src/solvers/CNEMOEulerSolver.cpp

+          // TODO: NOTE: Some of these are set above. They need to be set here as well, otherwise they are passed in incorrectly.
+          // Do not remove.
+


This is not something that just happens.
Most likely scenario is that you have a memory leak, which should be easy to find with valgrind.
Compile a debug or debugoptimized version, choose a small testcase that uses this feature, run
valgrind SU2_CFD your_config.cfg
see where it leads you.

Any luck @jtneedels ? We were planning to release 7.1.1 but we can also wait a little more.

@pcarruscag I haven't resolved it yet, but I am working on it more today. What is the timeline for the version 7.1.1 release? I don't want to hold it up.

We don't have strict deadlines but we try to have these maintenance releases every month.
This month is shorter, so if you want a specific day lets say Monday.

Okay sounds good. I'm definitely seeing a memory leak, and a number of uninitialized warnings in CNEMOEulerVariable.cpp and CNEMOEulerSolver.cpp. Still working on finding what's causing this specific issue.

Hi Pedro, I'm having trouble pinning down the memory leak causing the issue, but it looks like it might be coming from the Auxvar_grad terms used in the source residual. I'm having trouble getting specific line numbers when running valgrind with: valgrind --leak-check=full --track-origins=yes SU2_CFD testcase.cfg and compiling with debugoptimized. Is there anything I need to add to my run command?

debug instead of debug optimized might give more information... I do not know of anything else you may add.
Some of those warnings are infectious in that if something is not initialized all dependent quantities will also be a warning.
You need to start fixing from the top down. And then because of how the numerics (in general not just NEMO) are implemented the bug might be due to something else...

pcarruscag · 2021-03-01T11:24:14Z

SU2_CFD/src/numerics/NEMO/NEMO_sources.cpp

      residual[iSpecies] -= 0.0;
    residual[nSpecies] -= Volume*(yinv*total_viscosity_i*(PrimVar_Grad_i[nSpecies+2][1]+PrimVar_Grad_i[nSpecies+3][0])
                                                                    -TWO3*AuxVar_Grad_i[0][0]);
    residual[nSpecies+1] -= Volume*(yinv*total_viscosity_i*2*(PrimVar_Grad_i[nSpecies+3][1]-v*yinv)


... for example in line 365 above (sumJhs_y += (rho*Ds[iSpecies]*GV[RHOS_INDEX+iSpecies][1] - V_i[RHOS_INDEX+iSpecies]*Vector[1]) * hs[iSpecies];) and also 366, where is Vector[1] populated?

Its populated in CNEMONumerics.cpp, should I try repopulating it in NEMO_sources?

In GetViscousProjFlux? How does that value make its way into CSource_NEMO::ComputeAxisymmetric ?
Within each application of a public numerics method (like ComputeAxisymmetric or ComputeResidual) you need to be able to trace the result to the inputs, i.e. the things that are set into numerics from the outside before calling the method, or to the constructor, e.g. some constant that is stored in the constructor.
Otherwise you are computing things for one point based on values of another point.

This pattern we have in SU2 in general of "setting" followed by "compute" with very "stateful" objects (i.e. full of member variables used for intermediate results) is not very manageable unfortunately.
We have been getting away with it because most of these class hierarchies are simple, but for richer physics it is going to be headache after headache...

Yeah exactly in GetViscousProjFlux. So in cases like this, where there aren't set/compute functions I can just call to update the value, would the best remedy be to explicitly recompute Vector in CSource_NEMO::ComputeAxisymmetric?

Whenever you find yourself needing to do the same action in more than one location, make it a function.
Functions should have clear inputs and outputs.
Programming via side effects, i.e. you call a method of a class and it does some magic to a member variable that you use afterwards is a bad pattern.
The safeties the language offers to keep us from using that pattern are const and static qualifiers. const methods can only read from member variables, static can only use other static methods/variables.
"Make everything you can const" (every C++ author ever)

stale · 2021-06-02T16:07:45Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still a relevant issue please comment on it to restart the discussion. Thank you for your contributions.

fixed set functions for nemo axisymmetric, implemented auxvargrad cor…

5f4f7df

…rection in nemo axisymmetric source residual, fixed fluid model/transport model print statement Signed-off-by: jtneedels <jneedels@stanford.edu>

pr-triage bot added the PR: draft label Feb 24, 2021

jtneedels changed the title ~~fixed set functions for nemo axisymmetric, implemented auxvargrad cor…~~ Fixes to axi nemo set functions and auxvargrad in source residual Feb 24, 2021

jtneedels requested a review from WallyMaier February 24, 2021 02:22

jtneedels marked this pull request as ready for review February 24, 2021 04:07

pr-triage bot added PR: unreviewed and removed PR: draft labels Feb 24, 2021

WallyMaier reviewed Feb 24, 2021

View reviewed changes

adding change to print statement

b4b041a

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

WallyMaier reviewed Feb 24, 2021

View reviewed changes

fix sign on mass diffusion energy transport in source residual

af760c2

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

pcarruscag reviewed Feb 24, 2021

View reviewed changes

pcarruscag reviewed Mar 1, 2021

View reviewed changes

WallyMaier changed the title ~~Fixes to axi nemo set functions and auxvargrad in source residual~~ [WIP] Fixes to axi nemo set functions and auxvargrad in source residual Mar 26, 2021

pr-triage bot removed the PR: unreviewed label Mar 26, 2021

stale bot added the stale label Jun 2, 2021

stale bot closed this Jun 9, 2021

WallyMaier mentioned this pull request Jul 12, 2021

Fix for axisymmetric terms in NEMO + general NEMO updates #1326

Merged

5 tasks

WallyMaier deleted the feature_nemo_axi_fix branch July 12, 2021 22:09

		// TODO: NOTE: Some of these are set above. They need to be set here as well, otherwise they are passed in incorrectly.
		// Do not remove.

[WIP] Fixes to axi nemo set functions and auxvargrad in source residual #1205

[WIP] Fixes to axi nemo set functions and auxvargrad in source residual #1205

Uh oh!

Conversation

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

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

WallyMaier 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jun 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jtneedels commented Feb 24, 2021 •

edited

Loading