Skip to content

Conversation

@cvencro
Copy link
Contributor

@cvencro cvencro commented Apr 12, 2021

Proposed Changes

The multizone discrete adjoint driver is extended for dynamic problems.

A time loop is added to the multizone DA driver, with additional handling for the external container to store the intermediate values and cumulative sensitivities per time step.

@TobiKattmann - the changes to the DA multizone driver have been kept generic, they should be transferrable for CHT problems.

Adjoint gradients for both dynamic FSI and dynamic CHT have been verified against finite differences.

Dynamic adjoint test case has been added similar to an existing steady FSI test.

Related Work

#1259 (time averaged history output)
#1174 (velocity transfer at FSI boundary)

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 12, 2021
pcarruscag and others added 2 commits May 10, 2021 14:38
Co-authored-by: TobiKattmann <31306376+TobiKattmann@users.noreply.github.com>
@cvencro
Copy link
Contributor Author

cvencro commented May 11, 2021

A regression test is now added in the TestCases repo. Can we please merge the TestCase PR #62 first? Then we can uncomment the new test in parallel_regression_AD.py to include it in the checks.

@su2code su2code deleted a comment from lgtm-com bot May 18, 2021
@su2code su2code deleted a comment from lgtm-com bot May 18, 2021
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.

Ok I guess this PR is ready to merge then :) Lets go 🥳

* \param[in] force_writing - Force file output.
*/
void EvaluateSensitivities(unsigned long Iter, bool StopCalc);
void EvaluateSensitivities(unsigned long Iter, bool force_writing);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 StopCalc is a visible class boolean so it shouldn't be used as a member var in the same class anyway

iteration_container[iZone][INST_0]->Iterate(output_container[iZone], integration_container, geometry_container,
solver_container, numerics_container, config_container,
surface_movement, grid_movement, FFDBox, iZone, INST_0);
iteration_container[iZone][INST_0]->IterateDiscAdj(geometry_container, solver_container,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why "IterateDiscAdj" when its the Iterate of a discrete adjoint iteration class?

Copy link
Contributor

Choose a reason for hiding this comment

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

Citing @pcarruscag from his first review above. I also think it is reasonable to have this changed.

Maybe it would make sense to give CIteration an "Iterate" specific for adjoints, because the list of arguments is pretty long and CrossTerm does not mean much for primal solvers, and numerics, surface_movement, FFD and so on also do not make much sense for adjoints.

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to make more evident which "Iterate" should be used, and I thought it would be safer to have a new "Iterate" instead of an overload (which could be silently used somewhere else by mistake).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm ..
Agreed with the function arguments (though we could distinguish them just by different arguments). In my mind, the name indicates a discrete adjoint of the discrete adjoint. It's just a name though, if it's done very intentionally, I'm good with it.

Comment on lines +51 to +54
void CMeshVariable::Register_MeshCoord() {
for (unsigned long iPoint = 0; iPoint < nPoint; iPoint++)
for (unsigned long iDim = 0; iDim < nDim; iDim++)
AD::RegisterInput(Mesh_Coord(iPoint,iDim));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just being curious, how come that neither indices of updated mesh coordinates nor displacements (see above) are needed? I thought their adjoints must be initialised through at least one of them?

Copy link
Member

Choose a reason for hiding this comment

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

They are only used as inputs, but in #1284 I also changed this to store the input indices (just for consistency).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. :) Right, I didn't think of the discrete adjoint FEA solver for a second where the adjoint sweep goes through. I think I've worked out the precise initialize/extract logic for FSI couplings in one of your PR's one day but it's a while ago already...


unsigned long wrt_sol_freq = 9999;
const auto nOuterIter = driver_config->GetnOuter_Iter();
void CDiscAdjMultizoneDriver::KrylovInnerIters(unsigned short iZone) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Good to have this moved to a function, in this case.

@cvencro cvencro merged commit eedadbe into develop May 23, 2021
@cvencro cvencro deleted the feature_dynFSIAD branch May 23, 2021 21:47
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