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

Unsteady CHT #854

Merged
merged 33 commits into from
Feb 26, 2020
Merged

Unsteady CHT #854

merged 33 commits into from
Feb 26, 2020

Conversation

oleburghardt
Copy link
Contributor

@oleburghardt oleburghardt commented Jan 29, 2020

Proposed Changes

These are rather smaller changes and fixes so that the unsteady multizone framework can be used for CHT.

A test case together with a tutorial (mainly on how - and how easy it is - to turn a steady simulation into an unsteady one) will be added.

Related Work

Tutorial
Trailer on Youtube ;-)

@TobiKattmann @ScSteffen Please follow this so that we can set up Unsteady Adjoint CHT computations afterwards.

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.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

@pr-triage pr-triage bot added the PR: draft label Jan 29, 2020
Comment on lines 150 to 153
for (iZone = 0; iZone < nZone; iZone++) {
config_container[iZone]->SetDelta_UnstTimeND(config_container[ZONE_0]->GetDelta_UnstTimeND());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is ugly and if someone has got the time right now, I'd like to have your suggestions what the 'right' approach might be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's improved now by taking the minimum of all reference times as the reference time for all zones (not requiring any hard-coded values).

… options to turn it into its unsteady version).
@oleburghardt oleburghardt marked this pull request as ready for review February 7, 2020 14:12
@cvencro cvencro mentioned this pull request Feb 10, 2020
5 tasks
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

Have you checked that "suggest-parenthesis" warning? This would be a good PR to solve that.

Comment on lines 736 to 737
void Solve(COutput *output,
CIntegration ****integration,
Copy link
Member

Choose a reason for hiding this comment

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

this is one space out of alignment

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it looks good and it does not say 'Outdated' 😕 (concerning @pcarruscag 's comment)

Copy link
Member

Choose a reason for hiding this comment

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

void is 3 spaces from the margin 🤓

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.

Hi Ole,
thanks for this PR and especially for the tutorial and the video (that is also embedded in the tutorial-website) 💐 in my opinion this is really valuable

I have just a few comments below
(if you add the comments in the .cfg's please do that for the tutorial as well)

Cheers Tobi

@@ -131,4 +131,11 @@ class CMultizoneDriver : public CDriver {

bool Monitor(unsigned long TimeIter);

/*!
* \brief Returns wheter all specified windowed-time-averaged ouputs have been converged
Copy link
Contributor

Choose a reason for hiding this comment

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

whether typo

Comment on lines 736 to 737
void Solve(COutput *output,
CIntegration ****integration,
Copy link
Contributor

Choose a reason for hiding this comment

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

To me it looks good and it does not say 'Outdated' 😕 (concerning @pcarruscag 's comment)

@@ -747,19 +732,18 @@ class CHeatIteration : public CIteration {
* \param[in] surface_movement - Surface movement classes of the problem.
* \param[in] grid_movement - Volume grid movement classes of the problem.
* \param[in] FFDBox - FFD FFDBoxes of the problem.
* \param[in] val_iZone - zone of the problem.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why you removed val_iZone ? val_iInst is missing as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are missing everywhere. I think one should add those consistently.

* \author O. Burghardt
* \file CHeatSolver.hpp
* \brief Headers of the CHeatSolver class
* \author F. Palacios, T. Economon
Copy link
Contributor

Choose a reason for hiding this comment

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

Unimportant ... but why did you change the file author?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a very minor correction. Don't know why, maybe for consistency. There are no real guidelines for that anyway.

Copy link
Member

Choose a reason for hiding this comment

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

"If you wrote everything you are the author" than seems to be the rule no?

Comment on lines 286 to +288
/*--- Check whether the outer time integration has reached the final time ---*/

TimeConvergence = GetTimeConvergence();
Copy link
Contributor

Choose a reason for hiding this comment

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

..., or windowed time-average converged ---*/ as a comment enhancement suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ScSteffen Please check

Comment on lines 1235 to 1236
/*--- For steady-state flow simulations, we need to loop over ExtIter for the number of time steps ---*/
/*--- However, ExtIter is the number of FSI iterations, so nIntIter is used in this case ---*/
Copy link
Contributor

Choose a reason for hiding this comment

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

ExtIter is out of date, right? So I wonder whether this comment is still valid alltogehter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think CHeatIteration is not the best place to start with correcting such comments. E.g. CFluidIteration is the most important iteration class and practically one can adapt only from there to have everything aligned. But you're still right, so feel free to replace it if you like :-)

Comment on lines 29 to 35
%
%
TIME_DOMAIN = YES
%
%
TIME_MARCHING= DUAL_TIME_STEPPING-2ND_ORDER
%
Copy link
Contributor

Choose a reason for hiding this comment

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

As regression test cases and especially tutorial cases are probably early points of contact with people newer around SU2, I would like to have the config_template.cfg comment for the option pasted here (which is hopefully correct) and logically seperated like in the zone specific files. Like that is a little bit more self-explaining and displays possible alternative options.

But this comment is more of general nature

@oleburghardt
Copy link
Contributor Author

oleburghardt commented Feb 25, 2020

Have you checked that "suggest-parenthesis" warning? This would be a good PR to solve that.

@pcarruscag Are those displayed by -Wall? Just checked that in that case there are no such "suggest paranthesis" warnings.

@pcarruscag
Copy link
Member

I don't know what option that warning is included with, or if the behaviour is different across compiler versions.
I compile with with -Wall -Wextra, nevertheless please check the line I pointed to in your previous PR, I would gladly silence the warning but I don't know how that logic is supposed to work.

@oleburghardt
Copy link
Contributor Author

Ok. I'm still getting no warnings even with -Wall -Wextra.
But just to be sure I added some extra paranthesis around boolean operations in the code lines that you have referenced in my former PR.

@pcarruscag
Copy link
Member

👍 What compiler (and version) do you use?

@oleburghardt
Copy link
Contributor Author

gcc 9.2.1 :)

@pcarruscag
Copy link
Member

Here you go, Wall is enough.

@oleburghardt
Copy link
Contributor Author

Good to know, thanks! 👍 The problem must have been solved by someone else before then (the latest commit with extra paranthesis is not a correction in that regard).

@pcarruscag
Copy link
Member

Hmm, someone fixed it in this branch then, because develop still has it, anyway its fixed.

@oleburghardt
Copy link
Contributor Author

:-) Ok. Right know I'm trying to fix a bug for writing out surface sensitivities for the CHT cases in the repo. (It's probably not specific to CHT but a more general multizone problem.)

It would fit quite well here, so please hold on a second with merging!

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

@oleburghardt I'm merging this to resolve the conflicts with #861 and merge that one after.

@pcarruscag pcarruscag merged commit f1776de into develop Feb 26, 2020
@pcarruscag pcarruscag deleted the feature_unsteady_cht branch February 26, 2020 14:18
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.

5 participants