-
Notifications
You must be signed in to change notification settings - Fork 248
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
Kratos LSPG update. #11545
Kratos LSPG update. #11545
Conversation
Everything looks good, but I would suggest to rename the |
In Kratos, the naming conventions for solver settings have been a guiding factor for me. For example, when using the monotonicity preserving solver, there's a primary setting named However, understanding the desire for more specificity, I'd suggest considering one of these three options:
I'd like to also note my reservations about using the "&" character in the setting name, as it might introduce potential issues. |
@@ -39,7 +39,8 @@ def __init__(self, solver, custom_settings): | |||
self.solver = solver | |||
self.time_step_residual_matrix_container = [] | |||
self.echo_level = settings["echo_level"].GetInt() | |||
self.rom_settings = custom_settings["rom_settings"] | |||
self.rom_settings = custom_settings["rom_settings"].Clone() | |||
self.rom_settings.RemoveValue("inner_rom_settings") #Removing because the inner rom settings are specific for each builder and solver. |
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.
If they are specific to each b&s, let us reflect it on its name
I see your point now. However, I'd insist in changing the name to for example The proposals of keeping the |
@loumalouomega FYI, I changed some settings in the rom test that we had for the CoSimulation with ROM (NO CHANGES ON THE SOLUTION). Everything else is related to the ROM app. |
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 from my side
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.
LGTM
Pull Request Description
This pull request introduces significant updates to the LSPG builder and solver.
Main Changes:
Inheritance Update:
lspg_rom_builder_and_solver.h
inherited from elementalrom_builder_and_solver.h
.global_rom_builder_and_solver.h
which, in addition, inherits fromresidualbased_block_builder_and_solver.h
.Assembly Mechanism:
Solving Technique:
"qr_decomposition"
or"normal_equations"
for solving the rectangular system.Training the Petrov-Galerkin Model:
"residuals"
or"jacobian"
.Monotonicity Preserving Setting:
global_rom_builder_and_solver
andlspg_rom_builder_and_solver
inside the inner_rom_settings.ROM Manager Update:
Performance Enhancement:
Files to Review:
applications/RomApplication/custom_python/add_custom_strategies_to_python.cpp
: Exported functions to python.applications/RomApplication/custom_python/add_custom_utilities_to_python.cpp
: Bug fixes and removal of unused functions.applications/RomApplication/custom_strategies/global_rom_builder_and_solver.h
: Added inner_rom_settings, updated arguments, and changed variable access levels.applications/RomApplication/custom_strategies/lspg_rom_builder_and_solver.h
: Major changes here, including a shift from UBLAS to Eigen.applications/RomApplication/custom_utilities/rom_residuals_utility.h
: Removed unused functions.applications/RomApplication/python_scripts/calculate_rom_basis_output_process.py
: Updated settings.applications/RomApplication/python_scripts/hrom_training_utility.py
: Settings updates and validation.applications/RomApplication/python_scripts/petrov_galerkin_training_utility.py
: Bug fixes, code refactoring, and settings updates.applications/RomApplication/python_scripts/rom_analysis.py
: New settings assignment.applications/RomApplication/python_scripts/rom_manager.py
: Adapted for new settings.applications/RomApplication/python_scripts/rom_solver.py
: Assigned missing settings.Note:
Upon merging, the demo_rom_manager.py in KratosExamples will be immediately updated to reflect the new settings options.
Tests will undergo settings changes, but results are expected to remain consistent.
An error was identified in the thermal case (LSPG) related to the convergence criterion. The configuration of the test was set to the 'residual criterion'. However, because the previous LSPG B&S only accounted for the 'solution criterion' and not the 'residual', this led to the problem artificially and wrongly "converging" in just one iteration instead of the expected three. With the updated LSPG B&S, both 'residual' and 'solution' criteria are considered, thanks to the inheritance from the residual-based builder and solver. This change ensures a more accurate convergence assessment. Consequently, two thermal expected output binaries were updated to account for this change. It's worth noting that before this update, the new B&S passed the tests using only one iteration.
Example of new settings for an lspg:
Before:
Now: