Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "per-surface" outputs #1341

Merged
merged 8 commits into from
Aug 7, 2021
Merged

Fix "per-surface" outputs #1341

merged 8 commits into from
Aug 7, 2021

Conversation

pcarruscag
Copy link
Member

Fixes #1336

@pcarruscag pcarruscag linked an issue Jul 31, 2021 that may be closed by this pull request
@lgtm-com
Copy link

lgtm-com bot commented Aug 1, 2021

This pull request fixes 2 alerts when merging fa7f165 into f47e22b - view on LGTM.com

fixed alerts:

  • 2 for Comparison of narrow type with wide type in loop condition

Comment on lines -846 to +841
void CFlowOutput::WriteForcesBreakdown(CConfig *config, CGeometry *geometry, CSolver **solver_container){
void CFlowOutput::WriteForcesBreakdown(const CConfig* config, const CSolver* flow_solver) const {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of that last commit was trying to make this function less ugly.

@lgtm-com
Copy link

lgtm-com bot commented Aug 1, 2021

This pull request fixes 2 alerts when merging c43559e into f47e22b - view on LGTM.com

fixed alerts:

  • 2 for Comparison of narrow type with wide type in loop condition

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2021

This pull request fixes 2 alerts when merging 9577cba into f47e22b - view on LGTM.com

fixed alerts:

  • 2 for Comparison of narrow type with wide type in loop condition

Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well thanks for this fix(es) and massive cleanup 💐 Someone at some point will surely appreciate that tedious work!

Moreover, the "per surface" implementation is only considering the first marker name and value for printing.
However, there is one value added per "marker analyse".

Can you point me to where that went wrong real quick. I was not able to spot that while going over 😬

SU2_CFD/src/output/COutput.cpp Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Show resolved Hide resolved
Comment on lines +68 to +69
const bool si_units;
const bool us_units;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brief please

SU2_CFD/include/output/COutput.hpp Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Show resolved Hide resolved
}
}
(*convergenceTable) << out.str();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were re-using this string stream and that created the lumped values without separator.

case ScreenOutputFormat::FIXED:
PrintingToolbox::PrintScreenFixed(out, field.value, fieldWidth);
break;
case ScreenOutputFormat::SCIENTIFIC:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have made this block a lambda or something for re-use, one day...

@pcarruscag pcarruscag merged commit becab27 into develop Aug 7, 2021
@pcarruscag pcarruscag deleted the fix_per_surf_output branch August 7, 2021 08:22
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.

"Per surface" outputs do not work
2 participants