-
Notifications
You must be signed in to change notification settings - Fork 917
Reduce discrete adjoint memory usage ~25% #1159
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
Conversation
WallyMaier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcarruscag, this looks good to me! The code looks much cleaner, and bloated!
| /*--- Access the sub-volume of the element separately in 2D or 3D. ---*/ | ||
|
|
||
| su2double Volume_i, Volume_j; | ||
| switch (nDim) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a dumb question, but what motivates a switch here versus and if/else statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I'll change it there too.
To me a switch is useful to pick one out of many, it is also a good bug prevention tool for enumerator types as the compiler will warn if you forget to cover all the enum options (we convert our enum types to unsigned short which disables those warnings unfortunately).
| /*--- Compute the limiter in case we need it in the turbulence model or to limit the | ||
| * viscous terms (check this logic with JST and 2nd order turbulence model) ---*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to do what this comment had been asking (for quite some time), because I think it was possible to make the solver do unnecessary work e.g. compute limiters and reconstruction gradients for centered schemes.
Also there was some mixing between flow and continuous adjoint options, which looking at the adjoint solvers code seemed redundant (it never uses the flow solver limiter for example). The regressions seem to agree.
I tested the main changes of this PR on some 3D problems and all seems fine, so I'll merge this soon for the next release.
Proposed Changes
On the RANS onera m6 case (43k hex grid) the tape for the geometric recording becomes 13% smaller by:
On the hybrid grid for the same geometry we have in TestCases the reduction is 32%.
Some additional memory reduction is achieved by not storing the primal element face CG's and the edge CG's, these are only needed when building dual volumes and they are easy to recompute with the data already accessed where they are currently needed.
Related Work
#594
PR Checklist