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

Changed the registration of AD input variables in CDiscAdjFluidIteration #1492

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

thomasdick
Copy link
Contributor

This is a small issue I noticed when I worked on some features for the discrete adjoint driver.
In CDiscAdjFluidIteration::RegisterInput the inputs for the AD tape are registered.
At the moment if kind_recording == RECORDING::SOLUTION_AND_MESH is set it only registers the flow variables and has no difference to RECORDING::SOLUTION_VARIABLES. If I understand the logic for this enum option correctly, it should register the flow variables and the mesh coordinates from geometry. I.e. if you want to add unctions to the driver, which can evaluate derivatives w.r.t. flow and geometry in one tape evaluation, you should use this option.

In addition, I change an out-of-date comment.

Proposed Changes

Register the geometry variables as inputs in the discrete adjoint flow iteration if 'kind_recording == RECORDING::SOLUTION_AND_MESH'.

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

@thomasdick thomasdick changed the title Changed the registration of AD input variables in CDiscAdjFluidIterat… Changed the registration of AD input variables in CDiscAdjFluidIteration Jan 13, 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.

LGTM I don't think we are using that recording mode anywhere, might be a good candidate for removing / simplifying

@thomasdick
Copy link
Contributor Author

I checked and I could not find a place in the code where this option is used explicitly. Since all the drivers split between primary recording for state variables and secondary recording for geometry variables for efficiency.

My own use case was for research I did, where I needed to record a tape w.r.t. to both. Since this is not ready to become a pull request any time soon, I do not really need this option.

If you suggest I can create a commit to remove the enum option for simplicity.

@pcarruscag
Copy link
Member

If you plan to use it in the future let's keep it.

@pcarruscag pcarruscag merged commit b784edf into develop Jan 14, 2022
@pcarruscag pcarruscag deleted the feature_fix_recording_SOLUTION_AND_MESH branch January 14, 2022 23:42
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.

None yet

2 participants