-
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
Several features on tecplot io, slices, MG PC, cavitation constraints, and overset hole cutting. #231
Conversation
… chord for the moment computations
…rgence. also fixed the mgpc setup for ank turb ksp
f9bf2dd
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.
Very good additions! Just a few minor comments.
adflow/pyADflow.py
Outdated
# first, call the callback function with cgns zone name IDs. | ||
# this need to return us a dictionary with the surface mesh information, | ||
# as well as which blocks in the cgns mesh to include in the search | ||
surf_dict = explicitSurfaceCallback(self.CGNSZoneNameIDs) |
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.
For consistency and wherever possible, lets try to stick to camelCase here and elsewhere (many other locations)
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.
Do you mean we need to use camelCase even for variable names? Also, I think the recommendation in some styling guide was to use underscores within variable names and methods, and only camelCase in the class names. Obviously we are not using this convention. If you want me to change the pyADflow variable names to camelCase, I can make that change. Just wanted to confirm that this is what you want me to do.
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 think you are referring to is the python (or PEP8) style guide. Until we decide something else, we should be as consistent as possible with the current convention, which is camelCase for everything (both python and Fortran). I know its not perfect camelCase throughout, but we should make an effort to keep things consistent.
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.
okay, I will change the python to camelcase. Fortran is already not perfect and stuff like ANK and NK solvers always start with ANK_ or NK_. I prefer having underscores everywhere, but especially in fortran since it is not case-sensitive. For python, camelcase is fine. I will post and update when I complete the change.
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 changed all instances of underscore based naming that I could catch. This also requires a small PR on pygeo to be merged: mdolab/pygeo#169 please review and approve soon. I don't want to further delay this PR.
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.
Ok, I buy in some case its beneficial of having an underscore for readability. I haven't checked all fortran changes, but I am fine with the ANK_ etc. However, as a general rule, I think we should stick with camelCase.
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 looked through the code and I think there are still variables that could be changed to camelCase. For consistency, I would prefer that we follow the existing style in files that are updated. Also, with the python interface, the matching the variable name style would be great. Dont want to make this too big of a deal, perhaps others have a different option.
I addressed the low-hanging fruits @eirikurj. I have left a few questions for you to make sure I got them right. Once you confirm, I will make the remaining changes. |
@eirikurj, see this commit: 828f0b1 for the changes regarding to reducing code duplication for slices. At the end of the day, there's not that much code duplicated, so I decided to just gather them in a small preprocessing routine. All 3 slice routines do fairly different stuff, and I would either have if checks all over the place in a common routine, or reduce some functionality from the cylindrical slices. I think what I have right now is a good compromise. |
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.
@anilyil I am approving now since I think this is functionally very good and I dont want to delay this more. I think the code style could still be improved, but we can address this and others later.
Sounds good, I created an issue to keep track of the discussion: #252 I will review the code one last time myself and merge. |
Purpose
This branch of ADflow has some features I needed for different projects here and there, and the changes are getting out of hand. I will break down the changes to 3 large groups, and then there are some smaller fixes:
1. TecplotIO and Slices
These changes are in:
2. New CpMin Cost Function for Cavitation
I added a new way of do cavitation constraints, that use a KS-based aggregation to compute the minimum Cp over a given surface family. The true min for a surface family is computed by looping over its cells before the surface integration routine, and then the regular integration routine adds up the contributions. Unlike the previous cavitation constraints, this is conservative (i.e. the cpmin outputted will never be less than the true cpmin).
There is a new cost function that is called
cpmin
that outputs the KS-aggregated minimum Cp over a surface family. Similar to thecavitation
cost function, this is only computed when the ADflow optioncomputeCavitation
is set toTrue
. The default is false because at least thecpmin
computation introduces a global communication right before the surface integrations, which potentially slows the code down slightly.These changes are in:
3. New Overset Blanking Approach
Overset algorithm has a new way to do explicit blanking. With the new approach, users provide a closed surface (or a closed surface except for the symmetry plane) and then a list of the block IDs that we want to blank. Then in fortran, we figure out which cells in these blocks have any nodes that fall inside the provided surface. If any node is within the surface, we explicitly blank the cells. Multiple surfaces with different combinations of block IDs can be used. This approach helps a lot with cases where flooding is practically impossible to prevent due to the unique features on geometries. The "nodes in surface" approach is not exactly how flood seeds are computed; however, it is a good enough surrogate. The remaining cells may contain a handful flood seeds, but in the provided blocks at least, there should not be any more flooding happening since we explicitly blanked cells that are inside the surfaces. Ultimately, we also might want to have the option to disable the flooding in certain blocks, and the new flood seeds can just get tagged as explicitly blanked cells. The benefit of this is explicitly blanked cells force 2 fringe layers, so the algorithm knows which cells to try as interpolate first.
These changes are in:
Other Changes
In:
ANKPCUpdateCutoff
.In:
ANKUseApproxSA
.In:
In:
In:
In:
In:
Expected time until merged
Before we merge any of the fortran styling changes.
Type of change
Testing
I will try to add tests for individual features but not sure if I can cover them all.
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