New singlezone discrete adjoint driver #671

rsanfer · 2019-04-23T12:02:49Z

Proposed Changes

This PR goes in line with the driver reorganisation started in #585. At this time, it is the single-zone adjoint driver that is introduced, which inherits from the primal single-zone driver class. Problem properties are set at time of driver instantiation, and routines are mostly opaque to the physics (not yet completely as some further work is required in input/output). The functions are exposed to python so in a later PR the adjoint can also be used with the AD mode enabled. Some old driver classes are already removed.

Related Work

Further work by @oleburghardt to follow on the multizone adjoint driver.
Some features awaiting the input/output restructuring PR by @talbring.

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.

…h simple naca0012 steady-state case]

…not yet.

…eration is run now.

…_adjoint_single_zone

… the adjoint.

pcarruscag

Hi @rsanfer,
All looks fine, just a couple of questions.
Pedro

pcarruscag · 2019-04-30T12:51:22Z

TestCases/cont_adj_euler/wedge/of_grad_combo.dat.refdiscrete

-          1               , 0.0015213       , 0.001           
-          2               , 0.00177652      , 0.001           
-          3               , 0.00222003      , 0.001           
+          0               , 0.00651231      , 0.001           


Do you know why these have changed?

Yes, in fact I had to chase this one down for a bit. In principle this is a bug in the current code. It comes because the adaptive CFL is enabled and any time we do a recording after the first one (so, after there has been any geometry recording, for example) it uses the adapted CFL, so the path is changed a bit. With this version, that's no longer the case as the CFL adaptation is not called for the adjoint driver.

pcarruscag · 2019-04-30T12:54:34Z

SU2_CFD/src/driver_adjoint_singlezone.cpp

+  /*--- SetRecording stores the computational graph on one iteration of the direct problem. Calling it with NONE
+   *    as argument ensures that all information from a previous recording is removed. ---*/
+
+  SetRecording(NONE);


Is this required? I ask because it takes time (especially for FEA+Topology) and in the Adj FSI driver I have it commented out and all seems fine.

Actually, I have not tested it without, so it might be the case that it is not needed. Let me comment it out and see what it does.

I just tested it, flow adjoint tests are all failing
https://travis-ci.org/su2code/SU2/builds/526434537
so I reverted the changes. I think that @talbring can explain better why that's necessary.

Interesting, thanks for trying it. The FSI adjoint seems to work fine without it though, it definitely does not diverge, another AD mystery.

economon

Looking good @rsanfer. Just one or two requests and a couple of questions.

economon · 2019-05-01T16:09:43Z

SU2_CFD/include/driver_structure.hpp

+  CDiscAdjSinglezoneDriver(char* confFile,
+             unsigned short val_nZone,
+             unsigned short val_nDim,
+             bool val_periodic,


Can you please remove references to 'val_periodic' in all driver constructors? They were used before to check for periodic meshes so that the old CPhysicalGeometry() constructor could be instantiated. This is no longer necessary now that the periodic BCs have been fixed. That variable can simply be removed from the argument list and deleted.

Sure, I'll do that later today.I think it will need to be done also on the python scripts and so on.

economon · 2019-05-01T16:14:49Z

SU2_CFD/include/iteration_structure.hpp

@@ -1515,6 +1533,23 @@ class CDiscAdjFEAIteration : public CIteration {
                      unsigned short val_iInst,
                      unsigned short kind_recording);

+  /*!
+   * \brief Record a single iteration of the direct mean flow system.


Thanks for keeping the doxy up to date.. double-check some of these comments though (should mean flow -> elasticity here?)

Yep that's right, I missed these two. I'll update it.

economon · 2019-05-01T16:21:17Z

SU2_CFD/src/SU2_CFD.cpp

-      }
-
-    } else if (turbo) {
+    if (turbo) {


Will the remaining capability from all other drivers (turbo, HB, etc) be absorbed into the primal and adjoint single/multizone drivers such that they can be removed eventually?

That's the idea, to go towards a situation in which we only have Singlezone and Multizone Primal/Adjoint Drivers. The CFSIDriver will be removed in a subsequent PR (I didn't do it in this one as git was confused somehow) and the other drivers should also be removed and integrated into the new structures, which we should try to keep as constant as possible; but for that, we'd need some help from the community.

economon · 2019-05-01T16:32:54Z

SU2_CFD/src/driver_adjoint_singlezone.cpp

+  /*--- Specific scalar objective functions ---*/
+
+  switch (config->GetKind_Solver()) {
+  case EULER:                    case NAVIER_STOKES:                   case RANS:


You mention moving some items to the new output classes (I have seen this in the IO branch), but is it possible to move all of these switches and conditionals to the output too for holding the obj functions, such that one can simply call a virtual function in the base output class like output->SetObjFunc() here and not worry about the particular solver? We have discussed this before and maybe this is the longer term plan..

Yes, the idea is to create an intermediate class in charge of dealing with all these Postprocessing magnitudes, and return the ObjFunc required for each case. At this time I had to keep some if/switch statements at the driver, but our goal should be to reduce the number of them as much as possible and leverage on virtual functions. Some more work to come.

economon · 2019-05-01T16:36:39Z

SU2_CFD/src/driver_structure.cpp

@@ -4069,65 +4069,6 @@ void CDriver::Output(unsigned long ExtIter) {

 CDriver::~CDriver(void) {}

-CGeneralDriver::CGeneralDriver(char* confFile, unsigned short val_nZone,


Nice clean up.. feels good to remove lines :).

It really does!

economon · 2019-05-01T16:38:31Z

TestCases/disc_adj_euler/arina2k/Arina2KRS.cfg

@@ -48,6 +48,9 @@ DISCARD_INFILES= NO
 %                                       Speed = ft/s, Equiv. Area = ft^2 )
 SYSTEM_MEASUREMENTS= SI

+SINGLEZONE_DRIVER = YES


Is this option required or will it be deduced automatically?

I think that right now it would not even be necessary for adjoint cases, but for primal cases it is still needed. I would keep it there until we have completed the transition to ITER, TIME_ITER, and SINGLEZONE_DRIVER becomes the default for all cases.

pcarruscag · 2019-05-02T10:36:17Z

SU2_CFD/src/driver_adjoint_singlezone.cpp

+
+  /*--- Set the dependencies of the iteration ---*/
+
+  iteration->SetDependencies(solver_container, geometry_container, numerics_container, config_container, ZONE_0, INST_0, kind_recording);


I have a request, can you break some of these lines? I find myself horizontal scrolling too many times, maybe 120 characters long?

Sure, just limited the lines to 118 characters, which I think it's the limit of the github editor. Will push it in a bit.

rsanfer

Thanks for the comments, Tom! Individual replies below.

rsanfer · 2019-05-02T09:17:55Z

SU2_CFD/include/driver_structure.hpp

+  CDiscAdjSinglezoneDriver(char* confFile,
+             unsigned short val_nZone,
+             unsigned short val_nDim,
+             bool val_periodic,


Sure, I'll do that later today.I think it will need to be done also on the python scripts and so on.

rsanfer · 2019-05-02T09:20:41Z

SU2_CFD/include/iteration_structure.hpp

@@ -1515,6 +1533,23 @@ class CDiscAdjFEAIteration : public CIteration {
                      unsigned short val_iInst,
                      unsigned short kind_recording);

+  /*!
+   * \brief Record a single iteration of the direct mean flow system.


Yep that's right, I missed these two. I'll update it.

rsanfer · 2019-05-02T09:23:20Z

SU2_CFD/src/SU2_CFD.cpp

-      }
-
-    } else if (turbo) {
+    if (turbo) {


That's the idea, to go towards a situation in which we only have Singlezone and Multizone Primal/Adjoint Drivers. The CFSIDriver will be removed in a subsequent PR (I didn't do it in this one as git was confused somehow) and the other drivers should also be removed and integrated into the new structures, which we should try to keep as constant as possible; but for that, we'd need some help from the community.

rsanfer · 2019-05-02T09:25:54Z

SU2_CFD/src/driver_adjoint_singlezone.cpp

+  /*--- Specific scalar objective functions ---*/
+
+  switch (config->GetKind_Solver()) {
+  case EULER:                    case NAVIER_STOKES:                   case RANS:


Yes, the idea is to create an intermediate class in charge of dealing with all these Postprocessing magnitudes, and return the ObjFunc required for each case. At this time I had to keep some if/switch statements at the driver, but our goal should be to reduce the number of them as much as possible and leverage on virtual functions. Some more work to come.

rsanfer · 2019-05-02T09:26:14Z

SU2_CFD/src/driver_structure.cpp

@@ -4069,65 +4069,6 @@ void CDriver::Output(unsigned long ExtIter) {

 CDriver::~CDriver(void) {}

-CGeneralDriver::CGeneralDriver(char* confFile, unsigned short val_nZone,


It really does!

rsanfer · 2019-05-02T09:28:38Z

TestCases/disc_adj_euler/arina2k/Arina2KRS.cfg

@@ -48,6 +48,9 @@ DISCARD_INFILES= NO
 %                                       Speed = ft/s, Equiv. Area = ft^2 )
 SYSTEM_MEASUREMENTS= SI

+SINGLEZONE_DRIVER = YES


I think that right now it would not even be necessary for adjoint cases, but for primal cases it is still needed. I would keep it there until we have completed the transition to ITER, TIME_ITER, and SINGLEZONE_DRIVER becomes the default for all cases.

…_adjoint_single_zone

talbring · 2019-05-17T12:35:05Z

In general everything looks fine to me! I will do some smaller adjustments to this new driver for the new output structure soon (but in the other branch, not here). We can merge this in as long as there are no other comments.

pcarruscag

LGTM, thanks for the changes. No more comments on my side.

TobiKattmann

Hi Ruben, thanks for the effort. I have some (super) nitty-picky comments only except for the last one concerning the config_template.

SU2_CFD/include/driver_structure.hpp

SU2_CFD/src/driver_adjoint_singlezone.cpp

SU2_CFD/src/driver_structure.cpp

SU2_CFD/src/iteration_structure.cpp

TestCases/disc_adj_rans/cylinder_DT_1ST/cylinder.cfg

rsanfer

Hi @TobiKattmann,
thank you very much for the comments and for taking the time to go over the PR. I have just gone over the code and added the doxygen documentation and reformatting requested. This is very valuable to catch some comments that otherwise would stay on the code for long.
If there are no further comments to this PR, I will get it merged by tomorrow morning.
Best,
Ruben

SU2_CFD/include/driver_structure.hpp

SU2_CFD/src/driver_adjoint_singlezone.cpp

SU2_CFD/src/driver_structure.cpp

SU2_CFD/src/iteration_structure.cpp

TestCases/disc_adj_rans/cylinder_DT_1ST/cylinder.cfg

TobiKattmann

LGTM, I see you fixed some more comments than I found ;)
btw: I self-requested a review by accident and didn't know how to revert that but I wanted to approve anyway.

rsanfer added 30 commits April 4, 2019 11:22

Preliminary version of single-zone adjoint driver [compiles, runs wit…

f38dfeb

…h simple naca0012 steady-state case]

Minor change.

289e473

Enable time-domain adjoint.

d439118

Added inverse design routines to preprocess [should be only temporary]

93c1def

Modify travis to test the single-zone discrete adjoint driver.

1fcd260

Modify configuration files in the regression tests of discrete adjoint.

7871a70

Fix memory error.

6baddce

Homogeneous SetDependencies function in CIteration.

f6660c4

Enable singlezone output with FEA adjoint - nonlinear only, topology …

7bb0bc1

…not yet.

Enable topology optimization testcase. Tests will fail as only one it…

0f79cc5

…eration is run now.

Minor change so tests pass

dc6e436

Moved inv design routines to DiscAdjFlowPreprocessing.

02284e7

Revert travis.

3cd2add

Enable turbomachinery single-zone adjoint.

133a3f6

Fix bug in discrete adjoint heat, and enable singlezone driver

fb1affb

Modify disc adjoint heat testcase after bugfix.

db354c2

Remove CGeneralDriver.

fe4fcae

Remove CDiscAdjFluidDriver and childs.

62e3da4

Fix some compilation errors.

93dc3e0

Merge branch 'develop' of https://github.com/su2code/SU2 into feature…

3e7c4a2

…_adjoint_single_zone

Modify travis.yml.

e6b1ddd

Minor changes to test cases and bug fix.

eba35a7

Changes to regression cfgs.

60759da

Update iteration number in regression tests.

aeb86e0

Modify test case results, as CFL adaptivity should not play a role in…

b16daaa

… the adjoint.

change test case.

d1a07b9

Compute geometrical sensitivities at intermediate steps.

6fe7096

Fix spaces so diff works.

3a4e55c

Add option to record secondary variables.

03affaf

Revert travis.yml

337760c

Remove warning.

03a1a8b

rsanfer requested review from oleburghardt, economon and talbring April 23, 2019 12:02

Merge branch 'develop' into feature_adjoint_single_zone

6463746

pcarruscag reviewed Apr 30, 2019

View reviewed changes

rsanfer added 2 commits April 30, 2019 15:22

Test with NONE recording commented out

eae775d

Re-enable NONE recordings

e8b50dd

economon reviewed May 1, 2019

View reviewed changes

Fix some doxygen comments.

70310fc

pcarruscag reviewed May 2, 2019

View reviewed changes

rsanfer commented May 2, 2019

View reviewed changes

rsanfer and others added 6 commits May 2, 2019 13:42

Limit lines to 118 characters in singlezone_adjoint_driver.

46a37c4

Merge branch 'develop' of https://github.com/su2code/SU2 into feature…

8240032

…_adjoint_single_zone

Remove periodic bool from driver constructors.

b0645e5

Fix compilation error.

014c3d5

Merge branch 'develop' into feature_adjoint_single_zone

b66c6a5

Merge branch 'develop' into feature_adjoint_single_zone

a3f9e5c

pcarruscag approved these changes May 17, 2019

View reviewed changes

TobiKattmann reviewed May 17, 2019

View reviewed changes

Fixed comment issues.

beb6719

rsanfer commented May 28, 2019

View reviewed changes

TobiKattmann self-requested a review May 28, 2019 13:25

TobiKattmann approved these changes May 28, 2019

View reviewed changes

rsanfer merged commit 76fee99 into develop May 29, 2019

rsanfer deleted the feature_adjoint_single_zone branch September 17, 2019 14:02

talbring added the changelog:feature label Nov 7, 2019

talbring changed the title ~~Single-zone adjoint driver~~ New singlezone discrete adjoint driver Nov 8, 2019

pr-triage bot added the PR: merged label Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New singlezone discrete adjoint driver #671

New singlezone discrete adjoint driver #671

rsanfer commented Apr 23, 2019

pcarruscag left a comment

pcarruscag Apr 30, 2019

rsanfer Apr 30, 2019

pcarruscag Apr 30, 2019

rsanfer Apr 30, 2019

rsanfer Apr 30, 2019

pcarruscag May 2, 2019

economon left a comment

economon May 1, 2019

rsanfer May 2, 2019

economon May 1, 2019

rsanfer May 2, 2019

economon May 1, 2019

rsanfer May 2, 2019

economon May 1, 2019

rsanfer May 2, 2019

economon May 1, 2019

rsanfer May 2, 2019

economon May 1, 2019

rsanfer May 2, 2019

pcarruscag May 2, 2019

rsanfer May 2, 2019

rsanfer left a comment

rsanfer May 2, 2019

rsanfer May 2, 2019

rsanfer May 2, 2019

rsanfer May 2, 2019

rsanfer May 2, 2019

rsanfer May 2, 2019

talbring commented May 17, 2019

pcarruscag left a comment

TobiKattmann left a comment

rsanfer left a comment

TobiKattmann left a comment •

edited

Loading

		@@ -4069,65 +4069,6 @@ void CDriver::Output(unsigned long ExtIter) {

		CDriver::~CDriver(void) {}

		CGeneralDriver::CGeneralDriver(char* confFile, unsigned short val_nZone,


		/--- Set the dependencies of the iteration ---/

		iteration->SetDependencies(solver_container, geometry_container, numerics_container, config_container, ZONE_0, INST_0, kind_recording);

New singlezone discrete adjoint driver #671

New singlezone discrete adjoint driver #671

Conversation

rsanfer commented Apr 23, 2019

Proposed Changes

Related Work

PR Checklist

pcarruscag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

economon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talbring commented May 17, 2019

pcarruscag left a comment

Choose a reason for hiding this comment

TobiKattmann left a comment

Choose a reason for hiding this comment

rsanfer left a comment

Choose a reason for hiding this comment

TobiKattmann left a comment • edited Loading

Choose a reason for hiding this comment

TobiKattmann left a comment •

edited

Loading