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

Add center of force calculations #247

Merged
merged 115 commits into from
Feb 6, 2023
Merged

Add center of force calculations #247

merged 115 commits into from
Feb 6, 2023

Conversation

anilyil
Copy link
Contributor

@anilyil anilyil commented Nov 28, 2022

Purpose

This PR adds the center of force computations as ADflow cost functions. I am calling this center of force and not center of pressure because we look at the sum of all forces, which is what is more relevant for RANS solvers.

The center of force cannot be exactly determined from just the total forces and moments; however, the total moments can be computed from the center of force and total force calculations. However, I kept the moment computations as is and did not modify them. This is because the code already had a very good and granular implementation for the moment terms. It is also a good sanity check to compare the relationships between moment, total force, and center of force.

The center of force is returned in the following functionals:

cofxx
cofxy
cofxz
cofyx
cofyy
cofyz
cofzx
cofzy
cofzz

The first coordinate determines the force direction, and the second coordinate determines the component of the center of force. For example, cofxy is the y-coordinate of the center of x-force. It is up to the user how these are combined and used.

I also added a cost function called mavgvi, which is just a derived functional based on some compressible relationships. Not useful for most people.

Expected time until merged

Similar to #246, after #231 is merged, I need to update this PR. Currently it looks like there are tons of changes but in reality this is a standalone feature. I also need to add tests.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

anilyil and others added 30 commits January 30, 2022 20:07
…rgence. also fixed the mgpc setup for ank turb ksp
@anilyil anilyil requested review from lamkina and removed request for marcomangano and sseraj December 16, 2022 18:22
Copy link
Contributor

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

Was there a reason you didn't add a test for the mavgvi cost function as well?
Nevermind since this shouldn't be used much.

coFz = globalvals(coForceZ:coForceZ+2, :)

! before we go any further, compute the actual COP for each time spectral instance.
do sps=1, nTimeIntervalsSpectral
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you are doing an extra loop over the time spectral instances to compute the CoF values? It seems like they could be computed in the same loop that starts here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason I did this was because similar computations were happening there. To reduce the number of loops, I moved it inside the main loop. Current me also thinks this ended up being better.

@anilyil
Copy link
Contributor Author

anilyil commented Jan 22, 2023

@lamkina I also added tests for mavgvi. I test function value itself, and the adjoint sensitivity with the database, and the complex step derivative is checked against adjoint derivative in the database.

I have noticed we don't really have any adjoint tests for face integrated quantities other than the actuator zone tests. We may want to do a "test_functionals" like test for all face-integrated functionals down the line.

This PR is ready for another round.

@anilyil anilyil requested a review from sseraj January 22, 2023 15:40
@anilyil anilyil requested a review from lamkina January 22, 2023 19:27
lamkina
lamkina previously approved these changes Jan 24, 2023
Copy link
Contributor

@lamkina lamkina left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this looks good.

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Looks pretty good, but I have a few comments

tests/reg_tests/refs/actuator_tests.json Outdated Show resolved Hide resolved
src/modules/constants.F90 Outdated Show resolved Hide resolved
src/solver/surfaceIntegrations.F90 Outdated Show resolved Hide resolved
src/solver/surfaceIntegrations.F90 Outdated Show resolved Hide resolved
@anilyil
Copy link
Contributor Author

anilyil commented Feb 3, 2023

I think I addressed all of your comments @sseraj. Can you please take another look?

@anilyil anilyil requested a review from sseraj February 3, 2023 20:00
Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Looks good other than the flake8 check

@anilyil anilyil merged commit 2e4ce0e into mdolab:main Feb 6, 2023
@anilyil anilyil deleted the add_cop branch February 6, 2023 15:55
@gawng gawng mentioned this pull request May 24, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants