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

Restart Features for some python scripts #964

Merged
merged 32 commits into from
May 18, 2020

Conversation

ScSteffen
Copy link
Contributor

@ScSteffen ScSteffen commented Apr 30, 2020

Proposed Changes

  • Allows direct flow computations in shape_optimization.py to run from given restart solution for unsteady optimization.
    All you have to do is to copy your restart files e.g. restart_flow_00008.dat and restart_flow_00009.dat in the directory of your .cfg and .su2 (mesh) file and set the corresponding options:
    RESTART_SOL=YES
    RESTART_ITER=10 (in this example).
    The shape_optimization.py script will use this direct restart file for each design. This means, the direct computation will be started from iteration 10 and will run up to the final iteration. The adjoint will run from the final iteration, but stop at the iteration of the restart file.

  • direct_differentiation.py now also accepts restart files in the same manner.

  • SENSITIVITY is now a default field in VOLUME_OUTPUT, if the computation mode is DISCRETE_ADJOINT.

Related Work

Issue #909

PR Checklist

  • 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.

@ScSteffen ScSteffen requested a review from talbring April 30, 2020 14:46
@ScSteffen ScSteffen self-assigned this Apr 30, 2020
@ScSteffen ScSteffen marked this pull request as draft April 30, 2020 14:52
@ScSteffen ScSteffen changed the base branch from master to develop April 30, 2020 14:54
@pr-triage pr-triage bot added the PR: draft label Apr 30, 2020
@economon
Copy link
Member

economon commented May 8, 2020

Concerning this error (I am glad you brought it up):

Error in "virtual void CPhysicalGeometry::SetSensitivity(CConfig*)":

Sensitivity x not found in file.
------------------------------ Error Exit -------------------------------

Did you solve this issue? I think that the problem is related to specifying VOLUME_OUTPUT fields different than the default (or at least I have had one other person report this to me). When you put in custom fields, the sensitivities are not also added by automatically.

I think that we should enforce that the sensitivities are always written to the adjoint restart files, like we do with the coords and conservatives for the primal.

@ScSteffen
Copy link
Contributor Author

ScSteffen commented May 8, 2020

Hi @economon

Did you solve this issue? I think that the problem is related to specifying VOLUME_OUTPUT fields different than the default (or at least I have had one other person report this to me). When you put in custom fields, the sensitivities are not also added by automatically.

yes I noticed that adding sensitivity in the output solves the issue.
I agree, it makes sense to put SENSITIVITY as a default entry in the case of adjoints to avoid this problem.

@ScSteffen
Copy link
Contributor Author

ScSteffen commented May 9, 2020

I have a question regarding the failed test case:
opt_multiobj1surf_py using the config file inv_wedge_ROE_multiobj_1surf.
In this config, is the option RESTART_SOL=YES neccessary? There is no RESTART_ITER specified.
(Setting RESTART_SOL=NO fixes the failed test, however, I don't want to roam in other test cases without asking first ;) )

Edit: Fixed this issue without touching the test case. I got the point of this test case wrong :)...

@pcarruscag
Copy link
Member

That option has a default value, set somewhere in CConfig.cpp.

@ScSteffen
Copy link
Contributor Author

That option has a default value, set somewhere in CConfig.cpp.

I've added this in the last commit. :)

@ScSteffen ScSteffen changed the title Feature restart [WIP] Feature restart. May 12, 2020
@ScSteffen
Copy link
Contributor Author

I'm wondering, if a test case for unsteady restart shape optimization would be computationally too expensive for the test system.
Please let me know you opinion on that.
Otherwise this request is complete.

Best
Steffen

@pcarruscag pcarruscag marked this pull request as ready for review May 12, 2020 18:12
@pcarruscag
Copy link
Member

If you add it to the tutorials test suite... and it runs in < 5min, or it covers a few of the commonly untested options, I'd say go for it.

Out of curiosity, do you use the windowing feature to discard a few initial time steps?

Common/src/CConfig.cpp Outdated Show resolved Hide resolved
SU2_PY/SU2/eval/functions.py Outdated Show resolved Hide resolved
@ScSteffen
Copy link
Contributor Author

ScSteffen commented May 13, 2020

If you add it to the tutorials test suite... and it runs in < 5min, or it covers a few of the commonly untested options, I'd say go for it.

Ok!

Out of curiosity, do you use the windowing feature to discard a few initial time steps?

No not directly. The idea and motivation was, that I didn't want to wait until the transient phase of the flow field is computed. So I use a precomputed restart file, which serves as a better "inital guess" than freestream values. Then, so the idea, the transient phase is skipped for the first design, and it is at least shortened for other designs. This way, we safe CPU time.

@ScSteffen ScSteffen changed the title Feature restart. WIP Feature restart May 13, 2020
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.

It seems the test cases still runs in reasonable time so LGTM, give it a few days before merging in case the optimization people have something to say (I have no skin in this game as I don't use it).

@@ -62,6 +62,11 @@ CAdjElasticityOutput::CAdjElasticityOutput(CConfig *config, unsigned short nDim)
nRequestedVolumeFields = requestedVolumeFields.size();
}

if (find(requestedVolumeFields.begin(), requestedVolumeFields.end(), string("SENSITIVITY")) == requestedVolumeFields.end()) {
Copy link
Member

Choose a reason for hiding this comment

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

👍 I think this way of what happens in the output stays in the output is clearer, even if it requires duplicating a bit of code across the different adjoint output classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thanks for your feedback on this :)

@ScSteffen ScSteffen changed the title WIP Feature restart Restart Features for some python scripts May 15, 2020
@ScSteffen ScSteffen merged commit 8e4338e into su2code:develop May 18, 2020
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.

3 participants