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

Fix adjoint for streamwise periodic massflow + General handling of adjoints of additional solution variables #1536

Merged
merged 47 commits into from
Feb 26, 2022

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Feb 4, 2022

Dear reader,

Let me give a little bit background first:

When a massflow is prescribed for periodic flow (KIND_STREAMWISE_PERIODIC= MASSFLOW), then a pressure drop is iterated(/adapted in each iteration) to fulfill that massflow requirement. In the Volume Source term only this pressure drop is used. The initial value at solver start is the value given in STREAMWISE_PERIODIC_PRESSURE_DROP= ?? and for restarted/adjoint simulations the STREAMWISE_PERIODIC_PRESSURE_DROP from flow.meta is taken.

Now this PR:

Until now, the update of the pressure drop, based on the difference between current and prescribed massflow, was done in the Flow solver Preprocessing. That causes some trouble as that routine is called multiple times during normal starts, restarts and even twice(!) for every normal iteration. So do not put any iterated quantities in those like I did. I now did put the update right in front of where it is used -> in the Source_Residual. Like so there shoulnd't be that much trouble of correct taping in that one iteration for the adjoint and it makes the logic of Inner_iter/outer_iter==0 etc unnecessary.

I am not sure the changes up until now are necessary but I am not sure whether they are enough tbh. I will update this description accordingly:

Update: So to give you an idea of the current state of gradient validation. My setup is the good'ol 2D periodic Pin case (fluid-only) with the Design variables being FFD-parameters only around the middle pin (so no Periodic Interface shenanigans up to now) and I have 4 OF: Avg_Temp and Drag on the middle-Pin Surface as well as the Pressure-drop between In/outlet plus massflow through in- OR outlet (which is the same in value but maybe not for the gradient 🤔 ):

  1. For prescribed pressure drop I get a very good agreement for Avg_Temp and Drag (below 0.1% relative diff across the board).
    image
    For Massflow and pressure drop the gradients are off by quite a bit but not too shabby (a good optimizer certainly could make good use of those gradients). Of course the Pressure-drop gradient should be 0 but there are prob some numerical gains to be made. The Pressure-drop gradient is O(1e-5) where the others are O(1 - 1e4) so I consider that to be ok
  2. For prescribed massflow (and I differentiate here between the code before this PR and ...well this PR): The gradient for Avg_Temp and drag are okay-ish but far from being validated (just like for massflow and dp for prescribed dp)
    image
    For massflow OF I get wrongly reported gradients by the adjoints, both before and after this PR
    image
    and pressure drop OF
    image

Two other aspects.

  1. the PRESSURE_DROP OF is computed in streamwise periodic flow using the Recovered_Pressure (as the "standard" Pressure is the same on both faces). There I need to make sure that the recovered values are computed as well after the iteration (but with the Flow Preprocessing after the MultiGridCycle that seems to be the case)
  2. As pressure drop is iterated to get the correct flow solution, is it necessary to treat is specially in the adjoint like a solution variable? <- turns out, that was the case and also is the majority of work done in this PR

Le fin

Related Work

#773 initial implementation
#1527 first step in getting a consistent restart for massflow prescribed flow by storing the pressure drop in massflow-meta file

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

Before this update routine was called together with the NS Preprocessing.
This is troublesome as the Preprocessing is called multiple times during startup and
special precaution has to taken in order to avoid that the update happens multiple times (which,
by the way, happened anyway as the FlowPre is called before and after the MultiGridCycle).
Now in the Source Residual it is called directly before its usage which means no crazy logic is necessary
to prevent writing during multiple Pre steps and thus, makes sure that influence is taped during the adjoint as well.
The adjoint is still not working for a PressureDrop OF (the available one that is then computed
using the recovered Pressure which shouldn't be an issue because the Pre computes the latest one after
the iteration). Drag and AvgT look okayish but grad differences ~50% are to be expected.
@pr-triage pr-triage bot added the PR: draft label Feb 4, 2022
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.

Grabs 🍿

The fixed-Cl mode is not active during adjoints, the AOA is just taken from the metadata.
If something is determined iteratively, then I think it is a solution variable, but I never validated this. I always made my life simpler by constraining Cl instead of using fixed Cl 😊
In theory mass flow outlets and your periodic mass flow fall in the same category of fixed-Cl.

SU2_CFD/src/solvers/CIncEulerSolver.cpp Outdated Show resolved Hide resolved
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Feb 4, 2022

The fixed-Cl mode is not active during adjoints, the AOA is just taken from the metadata.

I.e. you would propose to try to disable the delta-pressure udpdate during the adjoint for prescribed massflow? In my understanding that would result in an inconsistent gradient -> which might work good enough for optimizations but in a gradient validation I would expect significant deviations.

If something is determined iteratively, then I think it is a solution variable, [...]

So it is settled then -> I will try to give pressure-drop the same treatment as regular solution variables as mentioned in point 2. of the PR description.

@TobiKattmann
Copy link
Contributor Author

In theory mass flow outlets and your periodic mass flow fall in the same category of fixed-Cl

I thought about that as well. If I get the streamwise periodic massflow to work that should be equivalent for massflow outlets. After all, quite a few ideas were taken from that implementation.

Maybe a question to @economon in that regard (I surely asked that already but cannot remember 🧠 🔨 ): Did you make a gradient validation for massflow-outlets back in the day? If yes, do you remember whether that went well, or did you see some imperfection that wasn't there for pressure-outlets?

@pcarruscag
Copy link
Member

Yes it is inconsistent that's why it would need validation, the worst-case scenario is when your design variable is the thing the primal solver is manipulating to implement the fixed mode.
For example I'm pitching the wing (geometric AOA let's call it) the adjoint gives me a value for dCL / dalpha, but in practice this value is 0 because the primal solver does not allow the CL to change.
But doing the above would be dumb right, the validation question is what happens to the derivatives of other outputs? Is dCD / dalpha correct? I imagine this depends on the relation between CD and CL in this case.

@pcarruscag
Copy link
Member

This can probably be tested with a dummy fixed point iteration...

As during the primal solution-finding process there is an iteration in the
pressure drop for prescribed massflow, the pressure drop should be treated like
the Solution as well. That means:
0. Storing of the restarted value to reset for the multiple recordings
1. Register Input and(!) output and therefore storage of the AD_Index
2. Set the AD-adjoint of the output with the Lagrange-Adjoint of the pressure drop (initVal 1e-16)
3. Extract the AD-adjoint of the input dp and set it as the Lagrange-Adjoint

I am not really set on how to treat multiple processes. Currently I just Allreduce
the local contributions at the AD-adjoint extraction. Needs testing.
But this version in single core is able to validate AvgTemp for single-zone massflow prescription
which wasn't possible before.
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Feb 6, 2022

With commit 1836f11 the gradients (for massflow prescribed in the primal) can now be validated really well (tested for AvgTemp and PRESSURE_DROP OF's).

BUT only for serial execution ... for multiple processes the adjoint diverges after a few iterations. I need to give that parallel treatment another thought.
UPDATE: With ce164a2 also the parallel execution works fine. Please read my explanation attempt in the commit message (in case you are interested) and let me know what you think

TobiKattmann and others added 3 commits February 6, 2022 15:59
Seemingly, the adjoint_dp can stay local to each core and does not need
to be accumulated across processes via Allreduce during adjoint extration.
This version works well with multiple processes and gives accurate gradientes against
FD.

I am not fully sure whether I understand why it works without communication.
1. The solution of course only really lives on 1 core as that is the point
of domain decomposition.
2. For fixed Variables like AoA, the allreduce is done, AoA of course equally lives on
all processes.
The streamwise periodic pressure drop also equally lives on all processes. But my guess, why
the extra comm is not needed is: The communication for dp is done in the primal solver. So
there the "link" between the dp's on all cores is made and is not required explicitely again. The
same argument would be used for the solution vars.
For the variables like AoA this is not the case. For the AD-tool, AoA really lives on each
process alone without all of them beeing linked, that is why the allreduce is necessary.
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Feb 7, 2022

So now that the Fluid-only version is reporting correct gradients for prescribed massflow due to the introduced adjoint equation for pressure drop -> the next problem seems to arrive: As of now I did not manage to get validated gradients for a CHT setup. I wonder whether sth additional is necessary there. But as the changes were introduced in the CDiscAdjSolver and CDiscFluidIteration ,always with using proper conditional to assure that the respective code is only excecuted when really needed, I am not sure if the mistake is not somewhere else.

Why cant shit just work 😞

btw: for prescribed pressure drop I get nice validated gradients for avgTemp, Drag, massflow

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Feb 8, 2022

Update/Solution: d42692f and 44bc57b add the Intermediate storage of the adjoint solution and the storage for the external(OF) contribution to the pressure drop adjoint which solves the issue described in this post 😊

So for CHT the situation is this:

For prescribed massflow I do get a good gradient agreement (DA vs FD). Below a picture with avgTemp and massflow OF
image

But if I switch to prescribed massflow and look at the avgTemp and pressureDrop OF then the gradient is again bad
image

Here an comparison of sensitivities (wrt to mdot and dp OF (top/bottom)) for massflow and pressure drop precsribed flows (red/orange). The expectation is, that for massflow prescription, the sensitivity wrt to the massflow OF would be zero ... but we see that the sensitivities are very similar to the pressure-drop prescribed flow, so the hypothesis now is, for multizone the algorithm for some reason does not incorporate the pressure-drop adjoint which works like a charm for singlezone
image

TobiKattmann and others added 7 commits February 8, 2022 12:30
For true multizone cases (nZone>1) the cross-term evaluation writes those sensitivities
into the Solution container. Therefore the "true" adjoint Solution needs to be
stored away intermediately in order to reset that after the cross-term eval.
That intermediate container ins Solution_BGS_k. The same concept in the same
places is here applied for Adjoint_DP. The code does the right thing but the sens
are still wrong so there has to be another mistake.

Testing is done with a single zone but MULTIZONE=YES but it doesnt work there either.
The df/ddp term seems to be the major one! So it needs an additional External-container
for the pressure drop adjoint.
@TobiKattmann
Copy link
Contributor Author

Now I am happy with the results of the code but the question now is: How to code this up nicely!
This concept of an additional iterated variable next to the solution variable is "new" to SU2.

  1. Add the Adjoint pressure-drop variables always to the existing arrays (Solution, External, AD_InputIndex, etc.) which would make for ugly allocation routines and one would have to mess with nPoint as one would prob extend the containers by one element. So I consider this direction to be a bit too intrusive as this concept is not needed for most applications anyway.
  2. Add an Extended part to all relevant copide methods and variables and keep that general for the different applications

@TobiKattmann TobiKattmann changed the title [WIP] Maybe fix adjoint for streamwise periodic massflow this time [WIP] ~~Maybe~~ fix adjoint for streamwise periodic massflow this time Feb 9, 2022
@TobiKattmann TobiKattmann changed the title [WIP] ~~Maybe~~ fix adjoint for streamwise periodic massflow this time [WIP] Fix adjoint for streamwise periodic massflow this time Feb 10, 2022
Currently the size of sol Extra is always 1 but maybe that changes in the near future
@TobiKattmann TobiKattmann marked this pull request as ready for review February 24, 2022 17:20
@TobiKattmann TobiKattmann changed the title [WIP] Fix adjoint for streamwise periodic massflow this time Fix adjoint for streamwise periodic massflow this time Feb 24, 2022
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.

👍 Looks quite simple to me now, what do you think?

SU2_CFD/include/solvers/CIncEulerSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CIncEulerSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CIncEulerSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CIncEulerSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
@TobiKattmann
Copy link
Contributor Author

Looks quite simple to me now, what do you think?

I fully agree. The CVariable footprint is much smaller and no more nasty address handling.

(Adding another variable in another solver requires a bit more code though and a little understanding of what is going on than the "Address"-version, but on the other hand this explicit handling of each primal-solver creates a good separation 👍 )

@pcarruscag pcarruscag changed the title Fix adjoint for streamwise periodic massflow this time Fix adjoint for streamwise periodic massflow Feb 25, 2022
@pcarruscag pcarruscag changed the title Fix adjoint for streamwise periodic massflow Fix adjoint for streamwise periodic massflow + General handling of adjoints of additional solution variables Feb 25, 2022
SU2_CFD/include/variables/CVariable.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CSolver.cpp Outdated Show resolved Hide resolved
@TobiKattmann
Copy link
Contributor Author

Thanks for the commit tackling the hardcoded allocation. And of course thanks for all the other suggestions and comments, made the code a lot better 💐 very much appreciated

Once the reg tests pass I'll merge this in 👍

@TobiKattmann TobiKattmann merged commit 3832a8f into develop Feb 26, 2022
@TobiKattmann TobiKattmann deleted the feature_StreamPer_massflow_PlzWörkDisTime branch February 26, 2022 09:52
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.

2 participants