Skip to content

Conversation

@oleburghardt
Copy link
Contributor

@oleburghardt oleburghardt commented Aug 26, 2019

Proposed Changes

This PR introduces the discrete adjoint counterpart for the multizone driver. It also comes with some heat solver-related additions for the new input/output structure as CHT (conjugate heat transfer) cases are the primary application so far.

The new driver adapts many of the well-known routines for registering variables, recording, initializing and extracting adjoints. The main difference is the handling of multiple zones.

The adjoint (fixed point-) iteration approach in CDiscAdjMultizoneDriver is to initialize them per-zone in an outer loop and to extract adjoints with respect to dependent variables from all zones, thereby inherently capturing the cross dependencies.
Multiple successive evaluations of adjoints with respect to the same zone (equivalent to inner iterations in the multizone driver) are possible.

In order not to have to re-record a tape in the (outer) zone-loop as it can be quite costly, this branch includes some new functionalities in the underlying AD-structure which make it possible to record a "full" tape that holds all derivative information and that can be accessed flexibly for different input and output variables across zones.
This also overcomes the restriction that adjoints have to be evaluated in the same order as their variables had been registered.

This is the first version of the driver and my intention is to continue contributing to it in terms of both performance and coverage of different kinds of physical couplings in SU2.

PR Checklist

A regression test for a heated cylinder array with conjugate heat transfer is added to TestCases/coupled_cht/disc_adj_incomp_2d.

The very same case is used in a tutorial showcasing the new driver and CHT sensitivity analysis, see Heated cylinders test case.

I'll check carefully for compiler warnings during test phase.

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

oleburghardt and others added 30 commits November 14, 2018 10:57
… coordinates within the CPoint class (one needs derivatives with respect to these e.g. in moving mesh mesh problems or in FSI).
@oleburghardt
Copy link
Contributor Author

Ok, I am ~halfway through and I have mostly nitpicking stuff. In general it is really tidy and I am looking forward to use everything you added. Rest will follow tomorrow.

Thanks a lot Tobias! I think I made all the changes that you suggested, it's just the ouput that you and Pedro do not like. I'll think about what can be done .. (though part of it might be due to the fact that the switch-thing is more apparent now, same thing can be found in the solvers as well).

I already did the tutorial you uploaded but I'll redo everything as I couldn't reproduce the nice gradients you show in the tutorial. I'll report to you... maybe I somewhere wrong

Could be please try to run SU2_DOT_AD in serial? (If this doesn't help and if you've got the time, also SU2_CFD_AD?)
That really must work .. it's quite important to me as I invested so much time so make it available for all. Could you send me your adjoint solution files?

@TobiKattmann
Copy link
Contributor

TobiKattmann commented Oct 9, 2019

Could be please try to run SU2_DOT_AD in serial? (If this doesn't help and if you've got the time, also SU2_CFD_AD?)
That really must work .. it's quite important to me as I invested so much time so make it available for all. Could you send me your adjoint solution files?

@oleburghardt Yep, Single core for both did the trick and I now get the same gradient results. I will double check with 28 cores to see if it still produces differing results.

@oleburghardt
Copy link
Contributor Author

@oleburghardt Yep, Single core for both did the trick and I now get the same gradient results. I will double check with 28 cores to see if it still produces differing results.

Do they really look different or do we rather see these kind of noisy spots where physical boundaries intersect with MPI boundaries? (In that case, it's likely that it's not an adjoint problem but that the driver computes accurate sensitivities based on a somewhere flawed primal code, i.e. the sensitivities are just unveiling a primal problem.)

Comment on lines +187 to +188
Donor_Variable[0] = Twall*donor_config->GetTemperature_Ref();
Donor_Variable[1] = heat_flux_density*donor_config->GetHeat_Flux_Ref();
Copy link
Contributor Author

@oleburghardt oleburghardt Oct 9, 2019

Choose a reason for hiding this comment

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

@economon Could you look over this and changes in the CHeatSolverFVM::Heat_Fluxes routine to check whether these usages of GetHeat_Flux_Ref meet your intention behind it?

Copy link
Member

Choose a reason for hiding this comment

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

See previous comment above..

Comment on lines +133 to +139
su2double Gamma = donor_config->GetGamma();
su2double Gas_Constant = donor_config->GetGas_ConstantND();
su2double Cp = (Gamma / (Gamma - 1.0)) * Gas_Constant;

Prandtl_Lam = donor_config->GetPrandtl_Lam();
laminar_viscosity = donor_solution->node[Point_Donor]->GetLaminarViscosity(); // TDE check for consistency
Cp = (Gamma / (Gamma - 1.0)) * Gas_Constant;
Copy link
Contributor Author

@oleburghardt oleburghardt Oct 9, 2019

Choose a reason for hiding this comment

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

@economon This is now completely analogue to the code in CNSSolver::Friction_Forces. Do you think it's okay that way?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it is best to have the flow solvers compute the information that you need here just once and store it (maybe we can use what is computed in Friction_Forces() or something similar), and only have the interface class access it from the corresponding solver (compressible vs incompressible). It could be dangerous to have two implementations of the same thing - if one changes and the other doesn't, the functionality breaks. It would be a lot easier to maintain, and simpler, to define it just once. Also, these quantities can be different between compressible and incompressible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's just about the compressible case.
For the incompressible one, I get a non-dimensionalized thermal conductivity from donor_solution->node[iPoint]->GetThermalConductivity() to then compute everything and finally use donor_config->GetHeat_Flux_Ref() to dimensionalize.
I am just not so much familiar with the compressible solver, but if it's sure that I could e.g. get the thermal conductivity always from FluidModel I could replace the computation (which is unsafe, as you pointed out) there by such a solver call.

@pcarruscag
Copy link
Member

@oleburghardt, looks like this is taking a turn to also address primal CHT topics, but all comments on the main dish have been addressed. Can we merge this to shorten the dependency chain between PR's while you continue working on ironing out minor kinks?

@oleburghardt
Copy link
Contributor Author

@oleburghardt, looks like this is taking a turn to also address primal CHT topics, but all comments on the main dish have been addressed. Can we merge this to shorten the dependency chain between PR's while you continue working on ironing out minor kinks?

True, but that was about it. They just have fit in so well. As for the main part, we just have to keep in mind the CDiscAdjMultizoneDriver::SetObjFunction that two of you didn't like. But as I said, it has much to do with other changes that should be done before.

@pcarruscag
Copy link
Member

Great! Let's merge then, I'll merge this with #753 today, please have a look at the changes affecting the new fields you added to CVariable on my next commit.

@pcarruscag pcarruscag merged commit b5ba8e6 into develop Oct 10, 2019
@pcarruscag pcarruscag deleted the sc_develop branch October 10, 2019 12:27
@pcarruscag pcarruscag restored the sc_develop branch October 10, 2019 12:27
}
else if (incompressible_flow) {

iPoint = donor_geometry->vertex[Marker_Donor][Vertex_Donor]->GetNode();
Copy link
Member

Choose a reason for hiding this comment

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

@oleburghardt just in the process of merging I think this line was removed by mistake? iPoint is used right after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a mistake. If you are in the merge window right now, can you revert that change/add that line in your branch? That should be the quickest solution to fix it in develop. Thanks in advance..

@talbring talbring mentioned this pull request Oct 10, 2019
3 tasks

/*--- Compute coupling between flow and turbulent equations ---*/

solver[iZone][iInst][MESH_0][FLOW_SOL]->Preprocessing(geometry[iZone][iInst][MESH_0], solver[iZone][iInst][MESH_0], config[iZone], MESH_0, NO_RK_ITER, RUNTIME_FLOW_SYS, true);
Copy link
Member

Choose a reason for hiding this comment

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

@oleburghardt this call is duplicated, can one of them be removed?

Copy link
Contributor Author

@oleburghardt oleburghardt Oct 11, 2019

Choose a reason for hiding this comment

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

I'll check and eventually delete it in #798 Thanks!

@talbring talbring changed the title Discrete adjoint multizone driver New Discrete Adjoint Multizone Capabilities (e.g. Adjoint CHT) Nov 8, 2019
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.

8 participants