-
Notifications
You must be signed in to change notification settings - Fork 99
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
Separation constraint using surface vector projection #238
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
==========================================
- Coverage 41.45% 41.07% -0.39%
==========================================
Files 13 13
Lines 4069 4119 +50
==========================================
+ Hits 1687 1692 +5
- Misses 2382 2427 +45 ☔ View full report in Codecov by Sentry. |
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.
The implementation looks good. I left some minor comments
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.
Thanks for addressing my comments. I have just a couple more
Thanks for the comments @sseraj |
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 have several detailed comments on the docstrings. Besides that, you should change code variables to use camelcase (e.g. sepsenmax_family should be changed to sepsenmaxFamily throughout).
doc/options.yaml
Outdated
sepSensorModel: | ||
desc: > | ||
This option allows to pick 3 separation sensor models. | ||
heaviside: Based on (Kenway2017b) |
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.
Also briefly explain this method like you explained the other 2 options.
doc/options.yaml
Outdated
@@ -1527,16 +1527,42 @@ verifyExtra: | |||
This option is for debugging the adjoint only. | |||
It is used to verify dIda. | |||
|
|||
sepSensorModel: | |||
desc: > | |||
This option allows to pick 3 separation sensor models. |
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.
change to "pick one of 3 separation sensor methods".
doc/options.yaml
Outdated
``0`` means no separation and ``1`` means that the flow is fully separated. | ||
Although this method is formulated for 3D cases, it is appropriate for airfoils. However, for 3D cases, please use ``surfvec_ks`` option, which has KS aggregation to aggregate the maximum separation sensor value to be constrained. This option will effectively alleviate separation anywhere on the interested surface and provide separation free design. On the other hand, ``surfvec`` option may not guarantee to achieve separation free design when the separation happens in a very smaller region and thus, the integration over this region may not violate the constraint. | ||
surfvec_ks: This method uses similar formulation to ``surfvec`` to compute the maximum separation sensor value on a desired surface instead of integrating over this surface. | ||
Therefore, this maximum separation sensor value from KS aggregation function can be constrained to alleviate separation. |
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.
change to "... function can be used to constrain separation."
doc/options.yaml
Outdated
desc: > | ||
This option allows to pick 3 separation sensor models. | ||
heaviside: Based on (Kenway2017b) | ||
surfvec: Surface vector projection. The separation is based on how much the local flow velocity deviates from the desired flow direction. |
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 have several problems with this explanation. First, the sweep angle is not explained. Why would we want to correct for sweep in the first place?
You say this is formulated for 3d, but applies to airfoils. That makes no sense. If it is only applicable to airfoils say so.
How the value varies from 0 to 1 is not well explained. Again, this is also with how the sweep is defined.
What is a common use case? Do you constrain separation to be below 0.0 or 0.5?
doc/options.yaml
Outdated
See "Buffet-Onset Constraint Formulation for Aerodynamic Shape Optimization" (Kenway2017b) for more details. | ||
|
||
sepSweepAngleCorrection: | ||
desc: > | ||
This option is only for ``surfvec`` and ``surfvec_ks`` separation models in ``sepSensorModel``. |
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.
what is the sweep direction? Do you account for fwd or bwd sweep? Do you account for lift direction being y or z?
This explanation needs to be a lot better. Right now it is not clear at all what it is doing.
Thanks for the comments @anilyil |
Purpose
This PR includes the new separation constraint in ADflow. The previous separation constraint and the new one is available. Each options can be selected through adflow option.
Expected time until merged
Type of change
Testing
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable