-
Notifications
You must be signed in to change notification settings - Fork 915
Fix #2483: Add field-based restart loading with missing field initialization #2675
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
base: develop
Are you sure you want to change the base?
Fix #2483: Add field-based restart loading with missing field initialization #2675
Conversation
| /*--- Load Pressure (index 0) ---*/ | ||
| if (fieldIdx_Pressure >= 0) { | ||
| solutionBuffer[0] = Restart_Data[baseIndex + fieldIdx_Pressure]; | ||
| } else { | ||
| /*--- Use default from config ---*/ | ||
| solutionBuffer[0] = config->GetPressure_FreeStreamND(); | ||
| if (rank == MASTER_NODE && counter == 0) { | ||
| cout << "WARNING: Pressure field not found in restart file, using config default." << endl; | ||
| } | ||
| } |
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.
Does the solver already have the default initialization when we load the restart?
If so, can we use the solution that is already in the solver nodes instead of repeating the logic for initialization?
If there is physics-specific logic required (incompressible, compressible, NEMO) it should be implemented in the respective solvers using e.g. a virtual function or static polymorphism based on the ENUM_REGIME template parameter of this class, instead of these long if/else blocks.
The names of the variables need to be in sync with those used for output. Therefore, they should be defined as constants and used in both places (output class and here).
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 the review. I have updated implementation:
- Defaults: Verified that nodes are pre-initialized. I removed the redundant default initialization logic, missing fields now simply preserve the existing values in the solver nodes.
- Polymorphism: Refactored
LoadRestartSolutionFieldsto use if constexpr on the ENUM_REGIME template parameter, ensuring clean static separation of physics logic. - Constants: Introduced a
RestartFieldNamesnamespace to ensure field names are consistent and defined in one place.
8488024 to
6f1f9ab
Compare
|
Nice! Can you add a testcase to the regressions (Testcases/parallel_regression.py + new config file), for instance one of the flat plate SA testcases or some testcase that already uses a restart file, and restart with SST instead? |
|
Added the requested regression test (turb_SST_flatplate_restart) to parallel_regression.py , which restarts an SA solution using the SST model. This verifies that missing turbulence fields are correctly initialized from defaults. |
| SurfaceViscCoeff.CEff[iMarker_Monitoring] = SurfaceViscCoeff.CL[iMarker_Monitoring] / (SurfaceViscCoeff.CD[iMarker_Monitoring] + EPS); | ||
| SurfaceViscCoeff.CEff[iMarker_Monitoring] = | ||
| SurfaceViscCoeff.CL[iMarker_Monitoring] / (SurfaceViscCoeff.CD[iMarker_Monitoring] + EPS); |
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.
revert all formatting changes to this file
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.
done
TestCases/parallel_regression.py
Outdated
| turb_SST_flatplate_restart.cfg_dir = "rans/flatplate" | ||
| turb_SST_flatplate_restart.cfg_file = "turb_SST_flatplate_restart.cfg" | ||
| turb_SST_flatplate_restart.test_iter = 10 | ||
| turb_SST_flatplate_restart.test_vals = [] |
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.
This cannot possibly test correctness if it doesn't have values
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.
done
Keep only functional changes for field-based restart loading. No formatting changes - file now has clean diff against upstream.
Remove multiple spaces before assignment operators to comply with PEP8.
| SOLVER= RANS | ||
| KIND_TURB_MODEL= SA |
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.
How was the restart file made? Which missing variables are tested here?
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 restart file is generated by euler_NACA0012_precursor.cfg (EULER solver) which outputs restart_flow_euler.dat containing only flow fields (Density, Momentum, Energy) but no turbulence variables.
The test then restarts using turb_SA_restart_test.cfg (RANS/SA solver) which loads this Euler restart file via SOLUTION_FILENAME= restart_flow_euler.dat.
Missing variable tested: Nu_Tilde (SA turbulence working variable) - which is absent in the Euler restart file.
The fix initializes the missing Nu_Tilde from freestream defaults (Solution_Inf), allowing successful Euler→RANS restart.
SU2_CFD/src/solvers/CTurbSolver.cpp
Outdated
| /*--- Determine number of turbulence variables in restart file ---*/ | ||
| /*--- Total vars in restart = coordinates + flow vars + turbulence vars + grid velocities ---*/ | ||
| unsigned short nVar_Restart = 0; | ||
| if (Restart_Vars[1] > skipVars) { | ||
| /*--- Estimate turbulence vars: total - skipVars, but cap at nVar ---*/ | ||
| nVar_Restart = min(static_cast<unsigned short>(Restart_Vars[1] - skipVars), nVar); | ||
| } | ||
|
|
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.
This cannot be done in a reliable way.
The best way forward I think is to introduce for each of the transported variables fixed variable names, the names that are now introduced only in CFlowOutput.
So for SA: "Nu_Tilde"
for SST: Turb_Kin_Energy, Omega
for LM: LM_gamma, LM_Re_t
We can then use these names in CFlowOutput and search for the presence of these strings in the header of the restart file.
You can then search independent of the number of fields (missing or not) that come before or after.
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.
Hi @bigfooted,
I have updated restart loading to use name-based field lookup instead of unreliable skipVars calculation.
- Added field name constants (
Nu_Tilde,Turb_Kin_Energy,Omega,LM_gamma, etc.) in CRestartFieldNames.hpp - SA/SST/LM models now use
FindFieldIndex(field_name)to search for variables without relying on field position - Missing fields are detected, a warning is issued, and defaults are used smoothly
- Removed unused
skipVarsfrom CFVMFlowSolverBase.inl
I tested this with turb_SA_restart_test.cfg (SA restart from Euler solution) and it correctly detected the missing Nu_Tilde, issued a warning, and ran successfully.
| @@ -0,0 +1,101 @@ | |||
| /*! | |||
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 don't think we need this file. It should be sufficient to add the field names to the solver class, for instance vector solverFieldNames={"Turb_Kin_Energy","Omega"} to CTurbSSTSolver.cpp.
… literals for restart field names in solvers
|
I don't see why you need most of the other changes. Let's first think about the use case: A user has a result without a turbulence model and now wants to add a turbulence model. Or a user wants to use the SST model instead of the SA model. I don't think we will have use cases where a user has a restart file with a missing flow variable. You can still use the same approach of getting the index from the location of the field names in the restart file, but we do not expect missing field variables here. The only thing you need to do for the ascii restart (let's focus on that one first) is read the first line of the restart file into a vector of strings and then check the indices of the fields in solverFieldNames for the respective solvers. So that's a function with like 5 lines of code (your lambda helper). Place this function centrally and re-use. Then just use these indices to read the data, when the index is valid. If it is not valid, you do nothing and the default data is used. |
…lver::FindFieldIndex
| void LoadRestartSolutionFields(CGeometry **geometry, CSolver ***solver, CConfig *config, | ||
| bool update_geo, bool static_fsi, bool steady_restart, | ||
| su2double* SolutionRestart, unsigned short nVar_Restart, | ||
| const string& restart_filename); |
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.
we don't need this. remove.
| */ | ||
| void LoadRestartGridData(CGeometry **geometry, long iPoint_Local, unsigned long baseIndex, | ||
| bool update_geo, bool static_fsi, bool steady_restart); | ||
|
|
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.
we don't need tihs. remove.
| struct TurbFieldInfo { | ||
| string name; | ||
| unsigned short model_var_index; | ||
| }; | ||
| vector<TurbFieldInfo> target_fields; | ||
|
|
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.
use vector<string>
| * \param[in] fieldName - Name of the field to find (with or without quotes). | ||
| * \return Index of the field (0-based, excluding PointID), or -1 if not found. | ||
| */ | ||
| int FindFieldIndex(const string& fieldName) const; |
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.
make input the vector of strings target_field, and output is vector of int with the indices.
| int CSolver::FindFieldIndex(const string& fieldName) const { | ||
| /*--- Search for the field name in the fields vector. | ||
| Fields are stored with quotes, e.g., "Temperature", so we need to check | ||
| both with and without quotes. The first field is "PointID" which we skip. ---*/ | ||
|
|
||
| string fieldNameWithQuotes = "\"" + fieldName + "\""; | ||
| string fieldNameNoQuotes = fieldName; | ||
|
|
||
| /*--- Search starting from index 1 (skip PointID) ---*/ | ||
| for (size_t i = 1; i < fields.size(); i++) { | ||
| string field = fields[i]; | ||
| PrintingToolbox::trim(field); | ||
|
|
||
| /*--- Check if field matches (with or without quotes) ---*/ | ||
| if (field == fieldNameWithQuotes || field == fieldNameNoQuotes) { | ||
| /*--- Return index excluding PointID (0-based) ---*/ | ||
| return static_cast<int>(i - 1); | ||
| } | ||
| } | ||
|
|
||
| /*--- Field not found ---*/ | ||
| return -1; | ||
| } | ||
|
|
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.
std::vector findIndices(const std::vectorstd::string& values, const std::vectorstd::string& searchvalues) {
std::vector indices;
for (const auto& search : searchvalues) {
auto it = std::find(values.begin(), values.end(), search);
indices.push_back(it != values.end() ? std::distance(values.begin(), it) : -1);
}
return indices;
}
| /*--- Fallback variables for legacy loading ---*/ | ||
| unsigned short skipVars = 0; | ||
| unsigned short nVar_Restart_Legacy = 0; |
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.
Remove legacy loading.
| #include "../../Common/include/CConfig.hpp" | ||
| #include "../../Common/include/geometry/CGeometry.hpp" |
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.
revert all changes to CFVMFlowSolverBase.inl. First implement only the missing restarts for turbulence solvers.
Proposed Changes
Adds field-based lookup for restart files, allowing SU2 to restart when required fields are missing (e.g., enabling the energy equation or turbulence model after an initial run without them). Missing fields are initialized from config defaults (e.g., temperature from INC_TEMPERATURE_INIT), with warnings. Maintains backward compatibility with standard restart files.
Key Implementation:
Related Work
Resolves #2483
Issue Description:
SU2 crashed when restarting a simulation with restart files that did not contain all necessary fields. For example, running an incompressible flow simulation without the energy equation, then enabling the energy equation, or enabling a turbulence model after an initial run without it.
PR Checklist
pre-commit run --allto format old commits.