-
Notifications
You must be signed in to change notification settings - Fork 249
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
[ROM] Unify ROM solver #9490
[ROM] Unify ROM solver #9490
Conversation
applications/ConvectionDiffusionApplication/python_scripts/convection_diffusion_solver.py
Outdated
Show resolved
Hide resolved
applications/FluidDynamicsApplication/python_scripts/adjoint_fluid_solver.py
Outdated
Show resolved
Hide resolved
applications/FluidDynamicsApplication/python_scripts/navier_stokes_compressible_solver.py
Outdated
Show resolved
Hide resolved
applications/FluidDynamicsApplication/python_scripts/fluid_solver.py
Outdated
Show resolved
Hide resolved
applications/RomApplication/python_scripts/new_python_solvers_wrapper_rom.py
Outdated
Show resolved
Hide resolved
I have a small question regarding the |
See my last comment regarding the |
@@ -0,0 +1,61 @@ | |||
import sys |
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 recommend coming up with a better name rather than prepending with new_
remember how long we were stuck with the new_linear_solver_factory
😅
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.
😄 Considering that this is a small research application I don't think we'll need to change keep backwards compatibility for a very long period of time. Indeed, the plan is to remove the old stuff ASAP. I'd keep the new_
to constantly remind us to do it 😄.
making rom_analysis compatible with empty "nodal_unknowns" list in "rom_parameters"
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 now
📝 Description
Similar to what we did in #9439 with the
RomAnalysisStage
, in this PR I'm unifying the ROM solvers in a newRomSolver
. The dynamic inheritance mechanism is equivalent to that of #9439. In order to keep backwards compatibility I kept the old solvers (adding a deprecation warning) and created anew_python_solvers_wrapper_rom.py
module. This module filters the base solver from which the ROM solver has to inherit.In this regard I'd like to note that, if we had a unified name for all the Python solvers wrapper (e.g.
python_solvers_wrapper
instead ofpython_solvers_wrapper_fluid
), the new ROM wrapper would be simplified and easier to maintain. In my opinion, it makes not so much sense to have different names for the Python solvers wrappers among applications if we follow the rule that one application should have only one "standard" wrapper (I said standard because of the adjoints). In any case this is to be discussed in an independent issue.This requires #9467 to be merged first.