Re-enable Discrete Adjoint FSI Test Cases

[rsanfer]

Proposed Changes

This PR is intended to put some functionality back that was involuntarily removed in #724.

Test cases for discrete adjoint FSI are now re-enabled with split files. They correctly pass the travis tests when the branch is based in feature_input_output, which was the original branch merged in #724.

Unfortunately, given that during this time both #753 and #774 were merged, the tests are failing now. Particularly, the disc_adj_fsi case runs into a segfault in CDiscAdjSolver::ExtractAdjoint_CrossTerm_Geometry, when trying to execute
geometry->node[iPoint]->GetAdjointCoord(Solution_Geometry);.

I believe this is related to the changes to the index based recording introduced in #774, as the code was running before into the same problem in CDiscAdjSolver::ExtractAdjoint_CrossTerm, and now goes beyond this point having changed the Get routine into direct_solver->GetNodes()->GetAdjointSolution_intIndexBased(iPoint,Solution);.

Currently, there is no GetAdjointCoord_intIndexBased or similar available, and its implementation requires some constructs for the indices. Therefore, I would like to ask for support from @oleburghardt and @pcarruscag to make this work with their changes.

Related Work

#724 involuntarily removed the discrete adjoint FSI cases from the regression tests.
Branch fix_discadjfsi successfully recovers the functionality removed.
#774 introduced relevant changes to the adjoint solver that are making.
#753 was merged and, although I tried to adapt to it, I can't guarantee it's working correctly.

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.

[pcarruscag]

Thank you for bringing this back. The testcase was passing on #753 alone, so that should not be the problem, I'll have a look.

[oleburghardt]

@rsanfer You shouldn't get in touch with the new AD routines at all unless you're using CDiscAdjMultizoneDriver.
So in case direct_solver->GetNodes()->GetAdjointSolution_intIndexBased(iPoint,Solution) gets executed, that likely means that the if-statement above, config->GetMultizone_Problem(), evaluates to true.
Some weeks ago I added another boolean (CConfig::GetMultiphysicsDiscrete_Adjoint()) but I was asked to revert that to the line above.

[oleburghardt]

Sure we do (e.g. CDiscAdjSolver::RegisterSolution will behave incorrectly).

So actually both ways are based on saving indices somewhere. However the routines that I added (with the _intIndexBased extension) do save them internally together with "their" corresponding variable (in the same variable class) which is important for the multizone stuff.

So now when we register the coordinates we do it index-based because the multizone_problem boolean is set.

We don't want the _intIndexBased routines for the FSI cases. They work by re-recording new tapes for each set of variables (fluid solution/coordinates/displacement solution) with added specific routines within the solvers/iterators for cross dependencies (which goes along with saving indices externally, but in a preassigned order).

What are the implications for when we extract the adjoints? Do we need an index-based GetDerivative?

Yes there are counterparts in RegisterSolution, SetAdjoint_Output and ExtractAdjoint_Solution. But nothing more. We can go for the internal indices in CVariable exclusively once we have integrated the FSI capabilities to the multiphysics driver.

[rsanfer]

@oleburghardt I figured why direct_solver->GetNodes()->GetAdjointSolution_intIndexBased(iPoint,Solution) was being executed, but as the FSI problem is multi-zone adjoint, it is required that config->GetMultizone_Problem() = true even when we are not using the driver CDiscAdjMultizoneDriver.

There is, of course, the possibility to set a specific boolean for these test cases in particular, but I think that would over-complicate the code. Else, it should be possible to extend the index based to the rest of the features required (geometry and structural solvers), which would be my preferred option.

[oleburghardt]

Ok. I don't think these changes are over-complicated.

But I'm fine with changing the index saving procedure everywhere. There'd be just some work to do (we would have to adapt everything for the FEA elasticity solvers and all the variables they need as well)... so it's also a question of time, testing and so on.

[rsanfer]

I think one option would add yet one more config option and make it more difficult for the user, and the other would be to extend new features to all existing capabilities of the code. The idea behind the single and multi-zone drivers was precisely generalization. In my opinion it would be a no brainer to go for the second, but I'll leave it up to the community, but it depends also a lot on what timing we are moving in.

[pcarruscag]

Is it possible for the fluid side to work index-based and the structural side as before?
We are combining the cross terms manually and re-recording each step anyway...

I solved the segfault by adding a GetAdjointCoord_intIndexBased() method to CPoint.
But the residuals of the flow adjoint are not changing, even on the first iteration of a problem with drag as objective function, I think something else is wrong.

[oleburghardt]

Is it possible for the fluid side to work index-based and the structural side as before?
We are combining the cross terms manually and re-recording each step anyway...

(Please rather say internal-based, or variable-based - both approaches are index-based, they differ in the way they are stored :-))

In principle, yes. One can have both at the same time. I'd have to think about it, sounds a bit messy to me right now to get it all consistent.

One could also change GetMultizone_Problem() to GetMultizone_Problem() && !GetFSI_Problem() or similar at those places. Anyway, we know the reason for this problem, so I'll leave it up to you which way we go.

but it depends also a lot on what timing we are moving in.

Yes.

[pcarruscag]

@rsanfer , @oleburghardt , I think I fixed it, I'll do further validation tomorrow, but the reference derivative is very close to the regression value.

The "int" in "intIndexBased" is for internal then? Because its type is also int, easy mistake to make xD.

[oleburghardt]

The "int" in "intIndexBased" is for internal then? Because its type is also int, easy mistake to make xD.

Yes.. The name was the first one I gave that routine. It somehow made it through.. Now that I had to type it several times I'd love to have it changed. But anyway..

I'm a bit puzzled that it seems to be so easy but maybe it's just as simple as you said - new approach inside CDiscAdjSolver and old in CDiscAdjFEASolver (if I got that correctly?). That would come in handy for all further developments. Let's wait for the validation. I'll also do one with this branch for the CHT adjoints tomorrow, just to be sure.

[pcarruscag]

Results are OK, I also checked I could replicate old results for a larger case.
I updated the reference results for my testcase, @rsanfer I suggest you have a look at the other one.

The only thing we needed was an internal index version of CPoint::GetAdjointCoord, nothing to do with #753.

Next time I see a PR with testcases commented out I think I'll close it xD

[rsanfer]

Thanks a lot for your support, @pcarruscag.

I run the other test cases this morning and I noticed there is a very small difference in the sensitivities around the 10th significant figure (in the order of the updates you made on the discrete adjoint airfoil case). Given that the order of magnitude of this difference remains consistent even when extending the simulation, I updated the test values. I leave here the reference to the previous state for our records.

If there are no comments against it in the next day or so, I will be merging in this PR next, as it just puts back some functionality that was removed.

[pcarruscag]

Does anyone know why we get this random failures from travis?

No output has been received in the last 20m0s, this potentially indicates a stalled build or something wrong with the build itself.

It does not seem to be related with a particular test case...

[talbring]

Does anyone know why we get this random failures from travis?

No output has been received in the last 20m0s, this potentially indicates a stalled build or something wrong with the build itself.

It does not seem to be related with a particular test case...

Unfortunately that happens a lot lately. Haven't figured out what the reason is. I assume its something with MPI on the virtual machines in Travis. Hard to debug though, I can test a different MPI version maybe...

[oleburghardt]

I'll also do one with this branch for the CHT adjoints tomorrow, just to be sure.

Just finished it. Everything's ok.