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.

Sign up for GitHub

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

Jump to bottom

Bugfix DiscAdjFluid: DualTimeStepping 1st order #554

Merged
merged 7 commits into from
Jul 24, 2018
Merged

Bugfix DiscAdjFluid: DualTimeStepping 1st order #554

merged 7 commits into from
Jul 24, 2018

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Jul 18, 2018 •
edited
Loading

Proposed Changes

Hi all,
there is a little bug in the loading of the primal solution for the DiscAdjFluid Iteration for DualTimeStepping 1st order.

  1. The solution for timestep n-1 should be loaded instead of n-2 -> I added an if statement there.
  2. solution_old is used as a temporary container for the freshly loaded solution. Then the solution which is currently at solution_time_n is pushed to be the "main" solution n+1. Afterwards the solution_old is put in the solution_time_n container -> That last step was wrong such that the loaded solution never gets involved.

I used the regression test in disc_adj_rans/cylinder which covers DualTimeStepping 2nd order and reused it for a new Testcase which covers 1st order.
For now, the .travis.yml has a different su2code/Testcases branch in it such that other are not hindered by this PR when running travis. If this PR is merged with the new Testcase, the TestCases repo has to be updated and the .travis.yml needs to be reverted.
Let me know if adding a new TestCase is OK. I am aware that not every detail can get its own regression test.

I checked my changes against finite differences for that Testcase. I'll post the result below.

Additionally I suppressed the output of .csv files in driver_structure.cpp for unsteady simulations if WRT_CSV_SOL= NO is set.

Regards, Tobi

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.
Nothing I know of.

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).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.

@TobiKattmann
Copy link
Contributor Author

As promised (drag) gradient comparison:
The blue curve represent the current develop branch with the bug. Bugfix and FinDiff are hopefully self-explanatory. The results are taken from my Testcase, so it is really small (10 timesteps), i.e. for a longer running case the gradient deviates a lot more.
Let me know if you want some other comparisons :)

image

image

rsanfer
rsanfer reviewed Jul 22, 2018
View reviewed changes
Copy link
Contributor

@rsanfer rsanfer left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM. Just let's get the test case in the TestCases repo first to avoid changes in .travis.yml

.travis.yml Outdated
@@ -80,7 +80,7 @@ install:

before_script:
# Get the test cases
- git clone -b develop https://github.com/su2code/TestCases.git ./TestData
- git clone -b feature_bugfix_DT1st_DiscAdjFluid https://github.com/su2code/TestCases.git ./TestData
Copy link
Contributor

Choose a reason for hiding this comment

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

You can do a pull request to the develop branch of the TestCases first to incorporate the new case.

SU2_CFD/src/driver_structure.cpp
@@ -3719,7 +3719,7 @@ void CDriver::Output(unsigned long ExtIter) {

/*--- Export Surface Solution File for Unsteady Simulations ---*/
/*--- When calculate mean/fluctuation option will be available, delete the following part ---*/
if ((config_container[ZONE_0]->GetUnsteady_Simulation() == DT_STEPPING_2ND) && (ExtIter % config_container[ZONE_0]->GetWrt_Surf_Freq_DualTime() == 0)) {
if ((config_container[ZONE_0]->GetUnsteady_Simulation() == DT_STEPPING_2ND) && (ExtIter % config_container[ZONE_0]->GetWrt_Surf_Freq_DualTime() == 0) && config_container[ZONE_0]->GetWrt_Csv_Sol()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, thanks! We'll need to rework some of this, but it's good that csv files will no longer be forced to be written.

@TobiKattmann TobiKattmann mentioned this pull request Jul 22, 2018
Merged
rsanfer
rsanfer approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@rsanfer rsanfer left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. I'll approve it now but I'll wait until tomorrow in case anyone has anything else to add.

economon
economon approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, Tobi.

Btw, can you confirm that there are no new warnings during compile?

talbring
talbring approved these changes Jul 23, 2018
View reviewed changes
Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks @TobiKattmann ! LGTM!

@rsanfer rsanfer merged commit 58892c9 into develop Jul 24, 2018
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Jul 24, 2018 •
edited
Loading

Hi @economon @rsanfer and @talbring ,

There are no additional warnings generated. In addition to a check of the build history in travis (@rsanfer gave me the hint and checked that too) (not including any AD-build), I compared the stderr-output of this branch 2fd2eb0 with the latest develop ed149f7. I used all the additional compiler flags and enabled mpi, autodiff and direct-diff and ran the build on a single core (i.e. make install 2> error.out 1> output.out). Otherwise (i.e. multiple cores) the order gets mixed up and it is impossible to compare the warning/error output.
A diff on the "error.out" of the two builds showed the files are identical (...like completely identical), although the files have ~18.000 lines. The stdout deviates only in the specified build dirs.

For now I checked the box above for this PR. If there's s.th. else to check let me know.

@economon I guess you're right in demanding to check the compiler warnings to ensure a nice code. Being strict in this regard probably helps everybody in the long run.

Thanks you all for the support although it is just a small contribution! It helps a lot.
Thanks! Tobi

@TobiKattmann TobiKattmann deleted the bugfix_DualTime1st_DiscAdj branch July 24, 2018 12:17
CatarinaGarbacz pushed a commit that referenced this pull request Aug 26, 2019
@rsanfer
Merge pull request #554 from su2code/bugfix_DualTime1st_DiscAdj
da96d8e 
Bugfix DiscAdjFluid: DualTimeStepping 1st order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@economon economon economon approved these changes

@talbring talbring talbring approved these changes

@rsanfer rsanfer rsanfer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TobiKattmann @economon @talbring @rsanfer