Skip to content

Conversation

@cvencro
Copy link
Contributor

@cvencro cvencro commented Apr 10, 2021

Proposed Changes

Improvements to time averaged history output for dynamic multizone problems, which now also include the structural zone. The averaging logic has been updated to include values only for the final solution in each time iteration.

To test the fixes in this PR, time averaged history output has been added to the existing dynamic fsi test case.

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.

@pr-triage pr-triage bot added the PR: draft label Apr 10, 2021
@pcarruscag
Copy link
Member

You could modify CWindowedAverage::addValue to only "push back" more values if it detects a change in current time iteration, otherwise it simple overwrites the last value in the history.
Then you could get rid of the entire logic in SetUpdate_Averages simply making it true or false (less logic is the way for less bugs).

@cvencro
Copy link
Contributor Author

cvencro commented Apr 13, 2021

Hi Pedro, thanks for the idea to update the windowing directly! I've updated addValue such that the values are added for new time only (replaces existing values if it is still the same time iteration). This is a lot simpler and very happy to remove the convoluted logic. The SetUpdate_Averages was still necessary though to pass the regression test for unsteady_cylinder_windowed_average.

Comment on lines -234 to +216
}
OutputScreenAndHistory(config);
Copy link
Member

Choose a reason for hiding this comment

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

👍

bool CFlowCompOutput::SetUpdate_Averages(CConfig *config){

return true;
return (config->GetTime_Marching() != TIME_MARCHING::STEADY && (curInnerIter == config->GetnInner_Iter() - 1 || convergence));
Copy link
Member

@pcarruscag pcarruscag Apr 14, 2021

Choose a reason for hiding this comment

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

It is strange that this would break the testcase, I'll have a look.

Copy link
Member

Choose a reason for hiding this comment

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

Your initial change was correct, the logic was artificially delaying the update of the average to the end of the timestep, and the timedomain regressions check the first inner iteration by default.

@pcarruscag pcarruscag marked this pull request as ready for review April 16, 2021 09:44
@cvencro
Copy link
Contributor Author

cvencro commented Apr 17, 2021

Hi Pedro, thanks for updating the regression test and all the tidy up as well.

@pcarruscag pcarruscag merged commit bd5dde1 into develop Apr 18, 2021
@pcarruscag pcarruscag deleted the fix_tavg_output branch April 18, 2021 13:03
TobiKattmann added a commit that referenced this pull request Apr 28, 2021
@TobiKattmann
Copy link
Contributor

Hi @cvencro, thanks for those changes 💐 I added very small changes to another PR

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