Several features on tecplot io, slices, MG PC, cavitation constraints, and overset hole cutting.

[anilyil]

Purpose

This branch of ADflow has some features I needed for different projects here and there, and the changes are getting out of hand. Here is a list of changes:

1. TecplotIO is restructured a bit to calculate the sectional moment coefficients about the local slice's quarter chord. This will likely result in garbage numbers when slices contain two or more closed curves (instead of just a single airfoil section), but the code should run regardless. It looks like I defaulted to using quarter chord (xFrac=0.25) for these calculations. I can add this as an option in this PR.
2. pyADflow is modified to allow arbitrary slices of the geometry. On top of the default custom family name and direction, users can also provide points and normal vectors that define the planes they want to slice through. An example use case of this involves getting the points and normals out of a pygeo reference axis (the axis's value and tangents evaluated at a range of spanwise locations will give the points and normals, respectively). This approach is useful when users want slices that is not exactly aligned with one coordinate frame, but maybe something that follows the dihedral of the wing.
3. pyADflow and TecplotIO is also modified to allow for cylindrical slices. With this approach, we pass another parameter that controls which side of the surface we should be slicing; If you have a rounded but hollow geometry like a nacelle, this approach can be used to define slices that propagate outwards from the center line of the nacelle. The code simply uses the same arbitrary slicing interface (point and normal), but also we need to provide which direction to pick. The surfaces going through the center axis will intersect a closed nacelle in 2 locations, and with the current implementation, only one side is kept, and this is controlled with the range of angles of the cylindrical slice.
4. The multigrid preconditioner was missing some calls that prevented its use with coupled ANK. These are fixed now. The PC is not that well tested still and its limitaations are not clear to me, but it does result in speedup with large cases. Coupled ANK variants work with the mgpc now.
5. 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). One major change is that the metric now outputs the minimum Cp, instead of a "cavitation sensor" value. Tapenade is rerun for this.
6. Overset initialization output is changed to have the connectivity errors printed optionally. I usually don't get too much value out of these since we have the cells with missing connectivities written to the volume CGNS. So now, unless the user asks for it, the connectivity errors are not printed.
7. 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.

Expected time until merged

Hopefully before we merge any of the fortran styling changes. A few weeks maybe?

Type of change

• [x] Bugfix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [x] 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

I will try to add tests for individual features but not sure if I can cover them all.

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

[codecov[bot]]

Codecov Report

Merging #231 (4a9be75) into main (b4ceb9a) will decrease coverage by 0.52%. The diff coverage is 30.00%.

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
- Coverage   41.30%   40.77%   -0.53%     
==========================================
  Files          13       13              
  Lines        3806     3882      +76     
==========================================
+ Hits         1572     1583      +11     
- Misses       2234     2299      +65     

Impacted Files	Coverage Δ
adflow/pyADflow.py	66.62% <30.00%> (-1.82%)	:arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[anilyil]

This is ready for the first round of reviews. 2 ADflow tests will fail once we merge https://github.com/mdolab/pyspline/pull/47. I want to re-train those tests in this PR as well, but first, we should merge the pyspline PR and then the pygeo PR at: https://github.com/mdolab/pygeo/pull/157

[sseraj]

To dos:

• [x] Add note on creating PLOT3D files
• [x] Update AVX config based on #228

[anilyil]

I think the AVX config can be updated later. Looks like that PR is still having some discussions in there. I will make the changes if it gets merged before this, or not make the changes if we merge this first. Other than this, this is good for a review.

@yqliaohk this PR adds another way to do the cavitation constraints. You can give it a try if you want. We find the minimum cp value over a surface family using KS aggregation. The difference compared to the previous formulation is that this one probably results in easier optimization problems because the function is less nonlinear, plus it is exactly conservative, the cp min output is always greater than the true cpmin, so you can have it set up so that you never violate the cpmin constraint for cavitation. It would be great if you can at least take a look at the changes in surfaceintegrations code in this PR.

@ArshSaja @lamkina In this PR, I also added a coordinate transformation callback function to any interactions with plot3d files. So scaling or translating the loaded coordinates is possible now. We can maintain one set of plot3d surfaces for your cases and move or scale them based on need, instead of maintaining multiple plot3d files for each configuration. Just a heads up.

[lamkina]

@anilyil I realized the cpmin cost function is only computed properly if the option computecavitation is set to True. There might be times when you only want cpmin and it might be confusing that you need to have the option comptuecaviation set to get that cost function to work.

[anilyil]

I think I addressed all of your individual comments @sseraj. I will look into adding the hole cutting test.

@lamkina, how do you suggest we approach this? Especially for cpmin, there's a flag to turn it on because it introduces extra computations that include global communications. I say we document this further in the functional docs and update the options.yaml file. I don't like adding more options that turn on individual functionals, and I doubt cpmin is useful for anything but cavitation computation.

[lamkina]

I think I addressed all of your individual comments @sseraj. I will look into adding the hole cutting test.

@lamkina, how do you suggest we approach this? Especially for cpmin, there's a flag to turn it on because it introduces extra computations that include global communications. I say we document this further in the functional docs and update the options.yaml file. I don't like adding more options that turn on individual functionals, and I doubt cpmin is useful for anything but cavitation computation.

As long as this is documented it should be fine. If this solution is an issue in the future we can come up with a different approach if necessary.

[anilyil]

I documented the behavior a bit better in the options.yaml file, and also changed the code slightly so that it returns zero when the option computeCavitation is set to False.

[gawng]

cavitation has been documented in #240

[anilyil]

I have updated the PR description to help reviewers. Do we want to have different people review different features, @eirikurj? I might have missed stuff, but I tried to point to where each change is happening in the code. I would really like to merge this PR this week.

[ArshSaja]

I will go over the cylindrical slice feature which I am currently using. I can also go through the other ones.

[anilyil]

Thanks for the review, @sseraj. I made the suggested change in the docstring so now the addPointset link appears in the sentence without breaking the flow.

I think one item left in your review is the test for the overset hole cutting. I might just turn it into an issue and get back to it once I have a good test case. I don't want to have this PR wait any further and it looks like I won't have time to add this test this week. It is a feature I will be using in the near future, so I am definitely interested in adding the test.

Other than this, did you have any other comments I did not address?

[sseraj]

Other than this, did you have any other comments I did not address?

Nothing else to address from me. I'm fine with adding a test later.

[anilyil]

As we have been talking to people, we figured it would probably be easier if different people reviewed different parts of this PR (sorry @sseraj).

@marcomangano, can you review the slice modifications and make sure I did not break Mads's stuff as we discussed? @ArshSaja, I modified the cylindrical slice code slightly. Can you please review the pyADflow and related fortran changes? @yqliaohk and @gang525, can you please take a look at the cpmin stuff? Just a quick look at at least the tests should be enough I think. @eirikurj can you take a look at the smaller changes I listed below? Especially, I wanted to add a new config.mk file that just adds some intel optimizations but these are hardware specific. Do you have a better solution here than to keep a separate file?

Regarding the overset changes, I think @sseraj and I have been using it and it works fine. I doubt it needs a strict review, but up to the reviewers. If someone wants to review it, I will say go ahead. If not, I am fine with merging this PR w/o that part of the code getting another review.

[anilyil]

What is the status on this PR? @eirikurj @marcomangano do you have any chance to take a look soon and approve the relevant parts to you?

[eirikurj]

@anilyil for now I had a look at the config, but I plan to have a quick glance over the entire thing later. In principle, its fine as is, just a separate file with the extra options. However, I wonder if we actually need a separate file. Why not just keep it in by default, as this has been supported by x86 processors for a while now? If we are worried, we can just do -axCORE-AVX2 instead of -xCORE-AVX2. What do you think?

[anilyil]

I am fine with keeping it in by default. the only problem is I think with KNL nodes, it may not work. Not sure about the details there, but I think the KNL stuff used a slightly different flag (ax might be enough for those, not sure). Another reason was I am not sure if we test with that optimization flag enabled, so it's more of a use it at your own risk thing.

Here's what I recommend, lets keep it as is in this PR, and then we create an issue. Once we progress with the intelOneAPI stuff on the docker side, we test with this compiler flag and see the results. I will also look into different hardware we are running at the time and then we can try to come up with a catch-all optimization flag. I mainly want to merge this one quickly before any of these efforts needs to happen.

[anilyil]

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.

[anilyil]

I created 2 issues that keeps track of the missing documentation/testing from this PR. The overset hole cutting in #251, and slice documentation in #250. These are to address comments by @sseraj and @eirikurj.

[anilyil]

@eirikurj, see this commit: https://github.com/mdolab/adflow/pull/231/commits/828f0b1e46be6f4e8d7be58852afc5c9cde44f56 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.

[anilyil]

Sounds good, I created an issue to keep track of the discussion: https://github.com/mdolab/adflow/issues/252

I will review the code one last time myself and merge.