Possible bug in Farfield BC implementation in SU2 v5

[SumanVajjala]

Hi. I think there is a bug in the Farfield BC implementation in SU2 v5. It is in the file solver_direct_mean.cpp at line number 10446 in the function CEulerSolver::BC_Far_Field. Basically, the boundary state is computed and then the residual is obtained as conv_numerics->ComputeResidual(Residual, Jacobian_i, Jacobian_j, config). Could this be a bug?

Shouldn't it be conv_numerics->GetInviscidProjFlux(&V_infty[nDim+2], Velocity, &Pressure, &V_infty[nDim+3], Normal, Residual).

Regards Suman Vajjala

[oleburghardt]

WHY should this be a bug? You don't provide any problems or erroneous solutions you might have had... This way you could just as well refer randomly to any other code line.

Possibly you are asking for whether that might be an improvement? (I don't think so. Although it might actually work to some extend, I see no point in breaking up the numerical scheme at certain surface elements... ? What is your argument?)

[SumanVajjala]

I said this could be a POSSIBLE bug. Shouldn't numerical flux (at farfield) be evaluated based on boundary variables calculated from the Riemann Invariants? Check the corresponding implementation for Euler_Wall and I think you'll understand why I thought it could be an issue! I think that is the standard implementation. Also when checking for subsonic/supersonic conditions i.e. |u.n+c| >= c or vice-versa you do not do it with freestream values. Please check lines 10368, 10378.

And I think this is my third interaction with the developers (I suppose you are one of the developers too). And let me guess......I havent opened any random file and a random line to report an issue.

Since you accuse me of randomly reporting issues which annoy you let me also tell you that there is a bug in your CUSP flux scheme implementation. Figure out what that is unless you also accuse me like the way you did. Hint: look at beta and Nu_c.

This will be my last interaction with you. You've taught me how your open-source environment works.

@fpalacios Please take note of the following discussion.

[oleburghardt]

Since you accuse me of randomly reporting issues (...)

No, absolutely not. But it makes the impression if you point to a line of code and then giving no information or ideas. Since no one of the core developers seemed to have a reply (and that might be the reason), I gave it a try ;).

Shouldn't numerical flux (at farfield) be evaluated based on boundary variables calculated from the Riemann Invariants?

It is.

[SumanVajjala]

So now please check why the flux is calculated as "conv_numerics->ComputeResidual(Residual, Jacobian_i, Jacobian_j, config)" instead of "conv_numerics->GetInviscidProjFlux(&V_infty[nDim+2], Velocity, &Pressure, &V_infty[nDim+3], Normal, Residual)" Why should Jacobian_j be involved? Unless you say that both the above routines give the same end result I am not convinced that it is right.

[ghost]

Dear @oleburghardt,

Unfortunately, your initial reply lacks of any constructive contribution.

SU2 depends on the feedback of you all. We should not discourage anybody to change/improve, show interest, ask for clarification, etc. The tone of your initial replay was unjustified and not polite. From now on, your profile as a member of the developer team (collaborator) will be not longer active.

Peace, Francisco

[oleburghardt]

Dear @fpalacios, it certainly wasn't my intention to hurt anyone's feelings or discourage him/her. My intention was quite the opposite since the issue remained unreplied for a few days - though I pointed out the reason too harsh, agreed.

[ghost]

Dear @oleburghardt. Thanks a lot for your reply,

As you know, We, as members of the "developers" team in the SU2 organization have more responsibility in SU2 than any other SU2 free developer around the world. This small and selected group (we are around 60 members) is the "official" external voice of the SU2 and we should try to be as polite, kind, patient and supportive as possible in any external communication.

We love SU2 and the open-source community is the cornerstone in its success. We must take care of the open-source community as he/she supports SU2. In fact, independently of the tone, I really appreciate that you used your knowledge and personal time to reply @SumanVajjala.

Thanks,

Francisco

On Fri, Jul 7, 2017 at 1:58 PM, oleburghardt notifications@github.com wrote:

Dear @fpalacios https://github.com/fpalacios, it certainly wasn't my intention to hurt anyone's feelings or discourage him/her. My intention was quite the opposite since the issue remained unreplied for a few days - though I pointed out the reason too harsh, agreed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/su2code/SU2/issues/405#issuecomment-313791399, or mute the thread https://github.com/notifications/unsubscribe-auth/AccuRpyJa9NMbjR7ncjCMArzr0SX-dtBks5sLpwIgaJpZM4OKV38 .

[hlkline]

Although my own participation with SU2 is now reduced due to pursuing other endeavors, I want to say that I hope that your future experiences with SU2 and the open-source community will be more positive, that I hope neither of you have been dissuaded from contributing here or on other open-source projects, and that this has been a good reminder that it is easy for things to get heated quickly, especially on the internet where tone of voice cannot be easily conveyed.

@SumanVajjala: conv_numerics→ComputeResidual is setting the fluxes inside of that function, and it uses the normal and flow variable information set at previous lines, conv_numerics→SetNormal(Normal); (line 10290 in your example) conv_numerics→SetPrimitive(V_domain, V_infty); (line 10433 in your example)

ComputeResidual can be found in numerics_direct_mean.cpp, where you will see that SetResidual is handled differently depending on the numerical scheme chosen, and that calls to GetInviscidProjFlux are included. A similar pattern is followed in the other boundary conditions, although there may be more going on here which someone else may be able to comment on.

If you have any questions that are not bugs/code issues, SU2 also has a forum at: https://www.cfd-online.com/Forums/su2/. I also recommend using an IDE (xcode, eclipse, etc) to assist with navigating the code.

Best, Heather

[SumanVajjala]

@hlkline Thank you for your reply. We have been using SU2 for over 2 years and only recently we started looking at the code to understand some of the issues we were facing. To clarify I did not report it as a bug. I do understand that the flow variables obtained by Riemann invariants (line 10433) are set as the primitive variables for the boundary point. But when it came to the flux calculation I saw Jacobian_j and that triggered my question. I know that each scheme has a ComputeResidual routine but for this specific case what routine is being used? Is it the one in numerics_template.cpp? And I still do not understand why Jacobian_j should play a role in the flux calculation! I will be glad if you can kindly comment on it.

Finally, can I now treat that this is a non-issue?

@fpalacios @oleburghardt @hlkline By the way don't you think Nu_c in line no 729 in numerics_direct_mean.cpp should be Nu_c = (1.0-Beta)LamdaNeg? This was the other probable* bug I was referring to within SU2 found in CUSP implementation.

Regards Suman

[ghost]

Thanks Suman,

My suggestion is the following… we have really never performed a detailed test of the CUSP scheme. As far I remember it was a really quick implementation.

And it looks to see that you know SU2 and you are ready to contribute … Would it be possible for you to create a pull request with the changes, including a test case? if it is a bug, your change will pay off immediately.

Best, Francisco

On Jul 12, 2017, at 10:03 PM, SumanVajjala notifications@github.com wrote:

@hlkline https://github.com/hlkline Thank you for your reply. We have been using SU2 for over 2 years and only recently we started looking at the code to understand some of the issues we were facing. To clarify I did not report it as a bug. I do understand that the flow variables obtained by Riemann invariants (line 10433) are set as the primitive variables for the boundary point. But when it came to the flux calculation I saw Jacobian_j and that triggered my question. I know that each scheme has a ComputeResidual routine but for this specific case what routine is being used? Is it the one in numerics_template.cpp? And I still do not understand why Jacobian_j should play a role in the flux calculation! I will be glad if you can kindly comment on it.

Finally, can I now treat that this is a non-issue?

@fpalacios https://github.com/fpalacios @oleburghardt https://github.com/oleburghardt @hlkline https://github.com/hlkline By the way don't you think Nu_c in line no 729 in numerics_direct_mean.cpp should be Nu_c = (1.0-Beta)*LamdaNeg? This was the other probable bug I was referring to within SU2 found in CUSP implementation.

Regards Suman

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/su2code/SU2/issues/405#issuecomment-314971522, or mute the thread https://github.com/notifications/unsubscribe-auth/AccuRkqESRvP7dcREH5t_PKPioRlkERhks5sNaUSgaJpZM4OKV38.

[WallyMaier]

The CUSP issue addressed in #705.