Skip to content

[WIP] Multizone adjoints for turbomachinery#2446

Open
joshkellyjak wants to merge 66 commits intodevelopfrom
feature_mz_turbomachinery_adjoint
Open

[WIP] Multizone adjoints for turbomachinery#2446
joshkellyjak wants to merge 66 commits intodevelopfrom
feature_mz_turbomachinery_adjoint

Conversation

@joshkellyjak
Copy link
Contributor

Proposed Changes

This is a cleaned up PR of the fixes needed for multizone adjoints for turbomachinery from the previous PR of @oleburghardt and I's work.

This hopefully is useable, so if you would like to test and report please feel free to contact me on Slack.

TODO:

  • Fix eddy viscosity tape issue in CNSSolver
  • Add turbomachinery objective functions + constraints
  • Include MZ testcase

Related Work

Now closed PR #2317

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • [ X ] I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • 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.

Comment on lines 675 to 677
AD::EndPreacc();

/*--- Max is not differentiable, so we not register them for preacc. ---*/
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between ending the preaccumulation before or after?

}
}
} else if (Multizone_Problem && DiscreteAdjoint) {
SU2_MPI::Error(string("OUTPUT_WRT_FREQ cannot be specified for this solver "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SU2_MPI::Error(string("OUTPUT_WRT_FREQ cannot be specified for this solver "
SU2_MPI::Error("OUTPUT_WRT_FREQ cannot be specified for this solver "

Comment on lines +3425 to +3426
"(writing of restart and sensitivity files not possible for multizone discrete adjoint during runtime yet).\n"
"Please remove this option from the config file, output files will be written when solver finalizes.\n"), CURRENT_FUNCTION);
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I'm pretty sure it is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one for @oleburghardt

Copy link
Contributor

@oleburghardt oleburghardt Mar 25, 2025

Choose a reason for hiding this comment

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

Yes and no, writing the files is fine, but the continuation of the multi-zone discrete adjoint solver is erratic. My guess is that some indices aren't cleared properly before re-recording the tape. (Writing adjoint solution variables to file only, without re-recording at all and without sensitivities, might give us some hints.)
The debug mode could eventually pin down where exactly things go wrong.

Comment on lines -222 to +229
BPressure = config->GetPressureOut_BC();
Temperature = config->GetTotalTemperatureIn_BC();

if (!reset){
AD::RegisterInput(BPressure);
AD::RegisterInput(Temperature);
}

BPressure = config->GetPressureOut_BC();
Temperature = config->GetTotalTemperatureIn_BC();

Copy link
Member

Choose a reason for hiding this comment

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

Why are these wrong but not the others that follow the same pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This threw a tagging error, and if I remember correctly it's because in the other sections the variables it accesses are directly used in the solver, whereas in the Giles implementation they are not, creating a mismatch in tags.

Copy link
Member

Choose a reason for hiding this comment

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

Can we fix it in a way that keeps a clear pattern for doing this type of thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can give it a go

… to run in quasi-MZ approach. (NEEDS CLEANING)
Comment on lines +138 to +201
switch(donor_config->GetKind_MixingPlaneInterface()){
case MATCHING:
targetSpan.donorSpan = iSpanTarget;
targetSpan.coefficient = 0.0;
break;

case NEAREST_SPAN:
// Find the nearest donor span
for (auto iSpanDonor = 0; iSpanDonor < nSpanDonor - 1; iSpanDonor++) {
const auto dist = abs(spanValuesTarget[iSpanTarget] - spanValuesDonor[iSpanDonor]);
if(dist < minDist){
minDist = dist;
tSpan = iSpanDonor;
}
}
targetSpan.donorSpan = tSpan;
targetSpan.coefficient = 0.0;
break;

case LINEAR_INTERPOLATION:
{
bool printWarning = false;
// Check if target span is within donor bound
if (spanValuesTarget[iSpanTarget] <= spanValuesDonor[0]) {
// Below hub - use hub value
targetSpan.donorSpan = 0;
targetSpan.coefficient = 0.0;
printWarning = true;
}
else if (spanValuesTarget[iSpanTarget] >= spanValuesDonor[nSpanDonor - 1]) {
// Above shroud - use shroud value
targetSpan.donorSpan = nSpanDonor - 1;
targetSpan.coefficient = 0.0;
printWarning = true;
}
else {
// Find the donor span interval that brackets the target span
for (auto iSpanDonor = 0; iSpanDonor < nSpanDonor - 1; iSpanDonor++) {
const auto test = abs(spanValuesTarget[iSpanTarget] - spanValuesDonor[iSpanDonor]);
if(test < minDist && spanValuesTarget[iSpanTarget] > spanValuesDonor[iSpanDonor]){
kSpan = iSpanDonor;
minDist = test;
}
}
// Calculate interpolation coefficient
coeff = (spanValuesTarget[iSpanTarget] - spanValuesDonor[kSpan]) /
(spanValuesDonor[kSpan + 1] - spanValuesDonor[kSpan]);
targetSpan.donorSpan = kSpan;
targetSpan.coefficient = coeff;
}
if (printWarning) {
if (rank == MASTER_NODE) {
cout << "Warning! Target spans exist outside the bounds of donor spans! Clamping interpolator..." << endl;
if (spanValuesTarget[iSpanTarget] <= spanValuesDonor[0]) cout << "This is an issue at the hub." << endl;
if (spanValuesTarget[iSpanTarget] >= spanValuesDonor[nSpanDonor - 1]) cout << "This is an issue at the shroud." << endl;
cout << "Setting coeff = 0.0 and transferring endwall value!" << endl;
}
}
break;
}
default:
SU2_MPI::Error("MixingPlane interface option not implemented yet", CURRENT_FUNCTION);
break;
}

Check notice

Code scanning / CodeQL

Long switch case Note

Switch has at least one case that is too long:
LINEAR_INTERPOLATION (40 lines)
.

Copilot Autofix

AI 7 days ago

In general, to fix a “long switch case” issue, you extract the lengthy logic into a dedicated helper function and have the case body invoke that function with the required inputs, returning or mutating data as needed. This shortens the case body, makes the switch easier to read, and isolates the complex behavior into a function that can be tested or modified independently.

Here, the best approach is to move the logic inside case LINEAR_INTERPOLATION: into a new private helper method of CMixingPlane, e.g. ComputeLinearInterpolationSpan(...). This helper will:

  • Take as parameters all the data it needs:
    • the target span index iSpanTarget,
    • the donor and target span values arrays spanValuesDonor and spanValuesTarget,
    • donor span count nSpanDonor,
    • the targetSpan struct reference (to write donorSpan and coefficient),
    • and rank (for warning output).
  • Implement exactly the same logic that is currently in the LINEAR_INTERPOLATION case: bounds checks with clamping, interval search, coefficient computation, and conditional warning printing.
  • Be defined as a member function in CMixingPlane.cpp (and declared in the corresponding header CMixingPlane.hpp, but we are not allowed to edit it here, so we will implement it as a static or anonymous‑namespace helper function in the .cpp instead, using only what we see). Since we cannot change the header, the safest is to define a static free helper function inside CMixingPlane.cpp near SetTransferCoeff and use it directly there.

However, we are constrained to only modify shown snippets and remain within Common/src/interface_interpolation/CMixingPlane.cpp. We can therefore:

  1. Introduce a static helper function above the switch within the same snippet, e.g.:

    static void ComputeLinearInterpolationSpan(
        int iSpanTarget,
        const su2double* spanValuesDonor,
        const su2double* spanValuesTarget,
        int nSpanDonor,
        int rank,
        decltype(targetSpans[0][0])& targetSpan);

    (With the exact implementation copied from the current LINEAR_INTERPOLATION case.)

  2. Replace the body of case LINEAR_INTERPOLATION: with a single call to that helper, passing in iSpanTarget, spanValuesDonor, spanValuesTarget, nSpanDonor, rank, and targetSpan.

All existing behavior (including the warning messages and value clamping) will remain identical, because we will not change the logic, only relocate it. No new imports or external dependencies are required.

Suggested changeset 1
Common/src/interface_interpolation/CMixingPlane.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/Common/src/interface_interpolation/CMixingPlane.cpp b/Common/src/interface_interpolation/CMixingPlane.cpp
--- a/Common/src/interface_interpolation/CMixingPlane.cpp
+++ b/Common/src/interface_interpolation/CMixingPlane.cpp
@@ -127,12 +127,56 @@
         targetSpans[iMarkerInt][nSpanTarget].donorSpan = nSpanDonor;
         targetSpans[iMarkerInt][nSpanTarget].coefficient = 0.0;
 
+        /* Helper for linear interpolation case to reduce switch-case length */
+        auto computeLinearInterpolationSpan =
+            [&](int iSpanTargetLocal,
+                decltype(targetSpans[iMarkerInt][iSpanTarget]) &targetSpanLocal) {
+                auto kSpan = 0; // Lower bound donor span for interpolation
+                su2double coeff = 0.0; // Interpolation coefficient
+                su2double minDistLocal = 10E+06;
+                bool printWarning = false;
+                // Check if target span is within donor bound
+                if (spanValuesTarget[iSpanTargetLocal] <= spanValuesDonor[0]) {
+                    // Below hub - use hub value
+                    targetSpanLocal.donorSpan = 0;
+                    targetSpanLocal.coefficient = 0.0;
+                    printWarning = true;
+                }
+                else if (spanValuesTarget[iSpanTargetLocal] >= spanValuesDonor[nSpanDonor - 1]) {
+                    // Above shroud - use shroud value
+                    targetSpanLocal.donorSpan = nSpanDonor - 1;
+                    targetSpanLocal.coefficient = 0.0;
+                    printWarning = true;
+                }
+                else {
+                    // Find the donor span interval that brackets the target span
+                    for (auto iSpanDonor = 0; iSpanDonor < nSpanDonor - 1; iSpanDonor++) {
+                        const auto test = abs(spanValuesTarget[iSpanTargetLocal] - spanValuesDonor[iSpanDonor]);
+                        if(test < minDistLocal && spanValuesTarget[iSpanTargetLocal] > spanValuesDonor[iSpanDonor]){
+                            kSpan = iSpanDonor;
+                            minDistLocal = test;
+                        }
+                    }
+                    // Calculate interpolation coefficient
+                    coeff = (spanValuesTarget[iSpanTargetLocal] - spanValuesDonor[kSpan]) /
+                            (spanValuesDonor[kSpan + 1] - spanValuesDonor[kSpan]);
+                    targetSpanLocal.donorSpan = kSpan;
+                    targetSpanLocal.coefficient = coeff;
+                }
+                if (printWarning) {
+                    if (rank == MASTER_NODE) {
+                        cout << "Warning! Target spans exist outside the bounds of donor spans! Clamping interpolator..." <<  endl;
+                        if (spanValuesTarget[iSpanTargetLocal] <= spanValuesDonor[0]) cout << "This is an issue at the hub." << endl;
+                        if (spanValuesTarget[iSpanTargetLocal] >= spanValuesDonor[nSpanDonor - 1]) cout << "This is an issue at the shroud." << endl;
+                        cout << "Setting coeff = 0.0 and transferring endwall value!" << endl;
+                    }
+                }
+            };
+
         for(auto iSpanTarget = 1; iSpanTarget < nSpanTarget - 1; iSpanTarget++){
             auto &targetSpan = targetSpans[iMarkerInt][iSpanTarget];
 
             auto tSpan = 0; // Nearest donor span index
-            auto kSpan = 0; // Lower bound donor span for interpolation
-            su2double coeff = 0.0; // Interpolation coefficient
             su2double minDist = 10E+06;
             
             switch(donor_config->GetKind_MixingPlaneInterface()){
@@ -155,46 +195,8 @@
                     break;
                     
                 case LINEAR_INTERPOLATION:
-                {
-                    bool printWarning = false;
-                    // Check if target span is within donor bound
-                    if (spanValuesTarget[iSpanTarget] <= spanValuesDonor[0]) {
-                        // Below hub - use hub value
-                        targetSpan.donorSpan = 0;
-                        targetSpan.coefficient = 0.0;
-                        printWarning = true;
-                    }
-                    else if (spanValuesTarget[iSpanTarget] >= spanValuesDonor[nSpanDonor - 1]) {
-                        // Above shroud - use shroud value
-                        targetSpan.donorSpan = nSpanDonor - 1;
-                        targetSpan.coefficient = 0.0;
-                        printWarning = true;
-                    }
-                    else {
-                        // Find the donor span interval that brackets the target span
-                        for (auto iSpanDonor = 0; iSpanDonor < nSpanDonor - 1; iSpanDonor++) {
-                            const auto test = abs(spanValuesTarget[iSpanTarget] - spanValuesDonor[iSpanDonor]);
-                            if(test < minDist && spanValuesTarget[iSpanTarget] > spanValuesDonor[iSpanDonor]){
-                                kSpan = iSpanDonor;
-                                minDist = test;
-                            }
-                        }
-                        // Calculate interpolation coefficient
-                        coeff = (spanValuesTarget[iSpanTarget] - spanValuesDonor[kSpan]) / 
-                                (spanValuesDonor[kSpan + 1] - spanValuesDonor[kSpan]);
-                        targetSpan.donorSpan = kSpan;
-                        targetSpan.coefficient = coeff;
-                    }
-                    if (printWarning) {
-                        if (rank == MASTER_NODE) {
-                            cout << "Warning! Target spans exist outside the bounds of donor spans! Clamping interpolator..." <<  endl;
-                            if (spanValuesTarget[iSpanTarget] <= spanValuesDonor[0]) cout << "This is an issue at the hub." << endl;
-                            if (spanValuesTarget[iSpanTarget] >= spanValuesDonor[nSpanDonor - 1]) cout << "This is an issue at the shroud." << endl;
-                            cout << "Setting coeff = 0.0 and transferring endwall value!" << endl;
-                        }
-                    }
+                    computeLinearInterpolationSpan(iSpanTarget, targetSpan);
                     break;
-                }
                 default:
                     SU2_MPI::Error("MixingPlane interface option not implemented yet", CURRENT_FUNCTION);
                     break;
EOF
Copilot is powered by AI and may make mistakes. Always verify output.
for (auto iSize = 0; iSize < size; iSize++){
if (buffDonorMarker[static_cast<size_t>(iSize) * static_cast<unsigned long>(nSpanDonor + 1)] != -1) {
for (auto iSpan = 0; iSpan < nSpanDonor + 1; iSpan++){
for (auto iVar = 0u; iVar < nMixingVars; iVar++) sendDonorVar[iSpan * nMixingVars + iVar] = buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars + iSpan * nMixingVars + iVar];

Check failure

Code scanning / CodeQL

Multiplication result converted to larger type High

Multiplication result may overflow 'int' before it is converted to 'unsigned long'.

Copilot Autofix

AI 7 days ago

In general, to avoid overflow when a multiplication result is intended to be stored in or used as a larger integer type, at least one operand should be explicitly cast to that larger type before the multiplication. This promotes the entire multiplication to be performed in the larger type, ensuring that intermediate results cannot overflow the smaller type.

In this specific case, the problematic expression is in the nested loop where values are copied from buffDonorVar into sendDonorVar:

for (auto iSpan = 0; iSpan < nSpanDonor + 1; iSpan++){
  for (auto iVar = 0u; iVar < nMixingVars; iVar++)
    sendDonorVar[iSpan * nMixingVars + iVar] =
      buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars +
                   iSpan * nMixingVars + iVar];
}

The product iSpan * nMixingVars is computed in the default integer type and then used as part of a size_t index expression on the right-hand side; a similar computation is used for the left-hand side index. The best fix is to promote iSpan (or nMixingVars) to a wide type such as size_t (or unsigned long, consistent with nearby code) before multiplication, ensuring that the multiplication happens in the wider type. Concretely, we should:

  • Cast iSpan to size_t (or unsigned long) in both index expressions where iSpan * nMixingVars appears.
  • Optionally, also cast nMixingVars to the same wide type for clarity and type consistency, but this is not strictly necessary; promoting just one operand is enough.

No new methods or external libraries are required; we only add explicit casts in the relevant expressions. All changes are confined to SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp in the shown region.

Suggested changeset 1
SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp b/SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp
--- a/SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp
+++ b/SU2_CFD/src/interfaces/cfd/CMixingPlaneInterface.cpp
@@ -106,7 +106,10 @@
     for (auto iSize = 0; iSize < size; iSize++){
       if (buffDonorMarker[static_cast<size_t>(iSize) * static_cast<unsigned long>(nSpanDonor + 1)] != -1) {
         for (auto iSpan = 0; iSpan < nSpanDonor + 1; iSpan++){
-          for (auto iVar = 0u; iVar < nMixingVars; iVar++) sendDonorVar[iSpan * nMixingVars + iVar] = buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars + iSpan * nMixingVars + iVar];
+          for (auto iVar = 0u; iVar < nMixingVars; iVar++)
+            sendDonorVar[static_cast<size_t>(iSpan) * nMixingVars + iVar] =
+              buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars +
+                           static_cast<size_t>(iSpan) * nMixingVars + iVar];
         }
         markDonor = buffDonorMarker[static_cast<unsigned long>(iSize) * static_cast<unsigned long>(nSpanDonor + 1)];
         break; // Avoid overwriting
EOF
@@ -106,7 +106,10 @@
for (auto iSize = 0; iSize < size; iSize++){
if (buffDonorMarker[static_cast<size_t>(iSize) * static_cast<unsigned long>(nSpanDonor + 1)] != -1) {
for (auto iSpan = 0; iSpan < nSpanDonor + 1; iSpan++){
for (auto iVar = 0u; iVar < nMixingVars; iVar++) sendDonorVar[iSpan * nMixingVars + iVar] = buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars + iSpan * nMixingVars + iVar];
for (auto iVar = 0u; iVar < nMixingVars; iVar++)
sendDonorVar[static_cast<size_t>(iSpan) * nMixingVars + iVar] =
buffDonorVar[static_cast<size_t>(iSize) * nSpanDonorVars +
static_cast<size_t>(iSpan) * nMixingVars + iVar];
}
markDonor = buffDonorMarker[static_cast<unsigned long>(iSize) * static_cast<unsigned long>(nSpanDonor + 1)];
break; // Avoid overwriting
Copilot is powered by AI and may make mistakes. Always verify output.
…n recording wrt mesh coordinates.

The tape debug mode code is currently also reworked in #2721. The changes in this commit could already be useful for the current turbo discrete adjoint development.
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.

3 participants