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

Refactorization of SU2 Grid Deformation for Python #1300

Merged
merged 86 commits into from
Feb 12, 2023

Conversation

patelha57
Copy link
Contributor

@patelha57 patelha57 commented Jun 10, 2021

Proposed Changes

This pull request represents progress towards exposing the mesh deformation routines and their adjoint counterparts to Python through pysu2 and pysu2ad, respectively. This refactorization introduced:

  1. CDriverBase
    a. Parent class for CDriver, CDeformationDriver, CDiscAdjDeformationDriver
    b. Common methods regarding geometry, markers, MPI, etc.

  2. CDeformationDriver and CDiscAdjDeformationDriver

In addition, the CDeformationDriver was modified to use the new mesh deformation solver CMeshSolver. The integration of CMeshSolver required introducing the numerics_container and solver_container, as well as minor changes to CMeshSolver (e.g., DeformMesh and SetMesh_Stiffness). Both the legacy mesh deformation solver SU2_DEF and the newer CMeshSolver can be used with these new drivers, depending on how the DEFORM_MESH option is set in the SU2 config file.

Related Work

These efforts are related to Issue #1262. The purpose of this work is to enable the coupling of SU2 with external solvers like Nastran, particularly for aerostructural MDAO. The goal is to apply this for coupled discrete adjoint sensitivities.

In the future, these driver classes will be extended to include more methods accessible via Python (i.e., for setting or extracting variables, config parameters, etc.). Currently CDiscAdjDeformationDriver is not integrated with CMeshSolver, but this will be addressed in the future.

PR Checklist

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

aa-g added 5 commits May 19, 2021 18:54
Refactor some pre-processing operations in constructor to Geometrical_Preprocessing() and Output_Preprocessing().
Refactor SU2_DEF main function to use the new CDeformationDriver class.
@lgtm-com
Copy link

lgtm-com bot commented Jun 10, 2021

This pull request introduces 10 alerts when merging 6facf5b into 919900a - view on LGTM.com

new alerts:

  • 10 for Resource not released in destructor

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.

Looking good 👍

SU2_DEF/include/drivers/CDiscAdjDeformationDriver.hpp Outdated Show resolved Hide resolved
SU2_DEF/src/meson.build Outdated Show resolved Hide resolved
SU2_DEF/src/drivers/CDiscAdjDeformationDriver.cpp Outdated Show resolved Hide resolved
SU2_DEF/src/drivers/CDiscAdjDeformationDriver.cpp Outdated Show resolved Hide resolved
SU2_DEF/src/drivers/CDiscAdjDeformationDriver.cpp Outdated Show resolved Hide resolved
@TobiKattmann
Copy link
Contributor

fyi

  1. The failing tutorials.py regression test are solved elsewhere so that will disappear upon the next pushed commit
  2. The Complex Code error of CodeFactor can be (most likely) disregarded as a false positive. It does so for eg CSolver.cpp already so i guess we have ourselves a new hypochondriac-file

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 12 alerts when merging c440148 into 350fee9 - view on LGTM.com

new alerts:

  • 12 for Resource not released in destructor

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 12 alerts when merging 169be9a into 350fee9 - view on LGTM.com

new alerts:

  • 12 for Resource not released in destructor

SU2_DEF/src/meson.build Outdated Show resolved Hide resolved
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@stale
Copy link

stale bot commented Sep 14, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still a relevant issue please comment on it to restart the discussion. Thank you for your contributions.

@pr-triage pr-triage bot removed the PR: draft label Jan 25, 2022
@patelha57
Copy link
Contributor Author

Okay the regression tests are all finally passing!
Can you advise on what to do about the CodeQL alerts? Any other comments or suggestions in general?

SU2_CFD/include/drivers/CDriver.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/drivers/CDriverBase.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/drivers/CDriverBase.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/drivers/CDriverBase.cpp Outdated Show resolved Hide resolved
@pcarruscag
Copy link
Member

You can leave the "uncontrolled path" warnings

@pcarruscag
Copy link
Member

I'll try to break down each request to make it clearer.

Request: Use nomenclature consistent with the C++ interface (point instead of node, etc.)
Reason: People who know the C++ interface will know how to use the python wrapper straight away, people who learn how to use the python wrapper will be able to understand the C++ implementation (without needing a translation layer in their mind all the time)

Request: Return connectivity/adjacency as local indices instead of global
Reason: This is the natural way of referencing data in an MPI code, local indices correspond directly to data stored in the partition thereby making it more efficient to access (without global-to-local conversions, which are much more expensive than local-to-global). Furthermore, the local ordering strategy improves the efficiency of loops (over edges, neighboring points, etc.)

Request: Do not apply unnecessary "transformations" to the connectivity/adjacency as part of the API
Reason: Increases the maintenance burden and makes for a less versatile API. By returning the local indices that form a marker or element, it is trivial to retrieve any other data for those indices (the "transformation"). For example, the solution at those points, the point coordinates, whether the points are halo or domain points, the global indices, etc.
This way, by adding a single function to the API e.g. get the global index of a point of element, the functionality of the API grows a lot more because the user can apply the function to any connectivity/adjacency function.

Request: Do not offer too many overloads of the same function
Reason: Increases the maintenance burden and it's returning the data in structures that are not very efficient, namely vector of vectors. It is much easier to offer domain-wide data access on the python side, as a very small function that can be part of wrapper utilities (i.e. live only in python but not c++). It may even be more efficient since you make a numpy array/matrix instead of lists of lists.

@pcarruscag
Copy link
Member

I've been testing this a bit this afternoon, things look ok.
We can revisit the accessor functions for the entire domain once we can return the container by reference instead of copying the data first.
We can also look into code-generating accessors for markers for whatever volume accessors we define, once the names stabilize a bit, there are some inconsistencies in the naming of the old python wrapper functions.

This needs regression tests now.

@pcarruscag pcarruscag merged commit 58cf2d4 into develop Feb 12, 2023
@pcarruscag pcarruscag deleted the feature_pysu2_DEF branch February 12, 2023 02:40
@RudiFeiman
Copy link

I'm trying to build SU2 with NVIDIA HPC compiler.
I found error with meson about dependency problem.
I found cond_config.hpp without NVIDIA HPC macro check. Does this mean SU2 didn't consider NVIDIA HPC compiler?

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

8 participants