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

Feature species - more adjoint parts #1461

Merged
merged 29 commits into from
Dec 10, 2021
Merged

Feature species - more adjoint parts #1461

merged 29 commits into from
Dec 10, 2021

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Dec 2, 2021

Hi,

Some necessary changes for the species transport solver to work nicely with the Discrete adjoint solver. So it is the same structure that was Introduced for CFlowOutput but now it is CAdjFlowOutput.

  • generalize turb+species output for Comp/Inc/NEMO away into a parent class to avoid duplication (Done)
  • Make More OF available such that one can actually use the DA solver
  • Add a Gradient validation + Optimization FADO template plus add DA regression tests (or tutorials 🤔 I think I will make a tutorial thing for this)
  • Some smaller fixes to the previous PR

Related Work

directly following #1388 where the majority of adjoint work for the species solver was already done

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.

@TobiKattmann TobiKattmann marked this pull request as draft December 2, 2021 17:10
@TobiKattmann TobiKattmann changed the title Feature species - more adjoint parts [WIP] Feature species - more adjoint parts Dec 2, 2021
@TobiKattmann
Copy link
Contributor Author

generalize turb+species output for Comp/Inc/NEMO away into a parent class to avoid duplication

Actually I just noticed there is no CAdjNEMOOutput or similar. Not sure @WallyMaier , but is the NEMO-tribe not doing in adjoints?

@pr-triage pr-triage bot added the PR: draft label Dec 2, 2021
@WallyMaier
Copy link
Contributor

@TobiKattmann I am actually developing the adjoint stuff for nemo right now! Though there are some recording issues....

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Dec 3, 2021

@TobiKattmann I am actually developing the adjoint stuff for nemo right now! Though there are some recording issues....

Ok @WallyMaier if this PR gets merged then there will be a CAdjFlowOutput class between COutput and e.g. CNEMOAdjOutput (or to be more precise: it is for inc and comp and it should be done for NEMO as well). That class provides output for the turbulence solvers and the separate species solver. So looking at CAdjFLowIncOutput then will show the methods called and that should be a straight forward change in `CNEMOAdjOutput

SU2_CFD/src/output/CAdjFlowOutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/CAdjFlowOutput.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

Really nice 👍 puts this logic into one place and avoids a bunch of work for future changes

Common/include/option_structure.hpp Show resolved Hide resolved
@TobiKattmann TobiKattmann marked this pull request as ready for review December 6, 2021 13:50
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Dec 6, 2021

I will add surface avg species_0 as another Objective function (Edit: done in 31d7a95) but that would be it from my side concerning this PR

Only Species_0 because one can of course always shift the species functions around. In case more than one avg surf Spec is needed at once, the one has to add it manually
@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Dec 6, 2021

So the merging process would of course be:

  1. merge tutorials (+website)
  2. revert regression.yml changes
  3. merge this PR

... in case the people are happy with it

SU2_CFD/include/solvers/CScalarSolver.inl Show resolved Hide resolved
SU2_CFD/include/solvers/CScalarSolver.inl Show resolved Hide resolved
SU2_CFD/include/solvers/CScalarSolver.inl Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CSpeciesSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSolver.cpp Outdated Show resolved Hide resolved
Note that the AddHistoryOutput lines are read by the
update_historyMap.py script. And if there are line breaks there are
information missing for that script.
Using // clang-format off/on can prevent this accidentally breaking.
SU2_CFD/src/output/CAdjFlowOutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/CFlowOutput.cpp Outdated Show resolved Hide resolved
TestCases/tutorials.py Show resolved Hide resolved
TestCases/parallel_regression.py Show resolved Hide resolved
SU2_CFD/src/solvers/CEulerSolver.cpp Outdated Show resolved Hide resolved
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@TobiKattmann TobiKattmann merged commit 1567259 into develop Dec 10, 2021
@TobiKattmann TobiKattmann deleted the feature_species branch December 10, 2021 10:47
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.

3 participants