[Fluid] Fine grained Python solvers #6575

rubenzorrilla · 2020-03-20T01:26:57Z

Dear @KratosMultiphysics/fluid-dynamics , @KratosMultiphysics/adjointfluid and @KratosMultiphysics/potential-flow-team ,

Finally I had some time to upgrade the Python solvers to be more fine grained. @philbucher as you are one of the promoters of this Python solvers design, I'd like you to review this.

The application tests work. I ran the GUI tests as well as the ones in the Examples repository. All of them work in serial and MPI

@philbucher I took the chance to do #6125

@sunethwarna I did also upgrade the adjoint solvers. However there are still some things to be improved and discussed (they are highlighted as # TODO). I think that there are enough changes in this PR so I'd leave these for another one. The tests pass but I didn't try the MPI solver. Can you please try it?

As a comment aside, I found that the turbulence model addition to the monolithic solver is way intrusive. I think that we need to discuss about this. As a temporary solution, the create_solution_scheme in the base solver check if there are turbulence models (something that I personally don't like too much...).

@adityaghantasala The Chimera solvers have been also updated. I think that the benefits of this design are evident in this case (thanks @philbucher for pushing it forward). The tests in the ChimeraApplication fail. One is because of the FS Chimera process (I think you missed a dot when naming the model parts). The monolithic one fails in the results check (I'm afraid I messed something...). Can you please have a look?

@marcnunezc and @inigolcanalejo I already updated the potential flow solvers.

Thanks.

(I take the chance to report that there are some MPI settings that yield a nan in the pressure convergence criteria. As this is already happening in the current master branch, I'd take care of it in a different PR).

closes #4390
closes #6125

rubenzorrilla · 2020-03-20T09:39:27Z

@marcnunezc and @inigolcanalejo can you please change the potential flow solvers accordingly.

I just realised that we need to do it in this same PR. I'll take care.

adityaghantasala · 2020-03-20T09:58:17Z

@rubenzorrilla Thank you very much for the changes ... I will go through the changes in the next days .. Also thank you for changing the Chimera scripts too ... I will make the necessary other changes and push them here ...

philbucher

Very nice work!
I think overall it goes in a good direction but I think there are still some flaws in the design, esp in how the settings are handled:

The baseclass has no settings yet it uses them! This makes it very hard to follow where what is defined
The trilinos solvers seem to duplicate pretty much all default settings from the (serial-) baseclass => I think this should be improved, check the structural solvers, I think we solved it in a good way
Consider refactoring some methods to an external utility, e.g. like this or this. This should clean up the baseclass a lot

Unfortunately there are also quite some cosmetic changes, e.g. in the logger or in the naming of existing functions. It would be very helpful from a review point of view if you could cherry-pick those if possible to a separate PR that goes in before this one

philbucher · 2020-03-27T11:25:45Z

...ations/StructuralMechanicsApplication/python_scripts/trilinos_structural_mechanics_solver.py

@@ -63,7 +63,7 @@ def Finalize(self):

    #### Specific internal functions ####

-    def get_epetra_communicator(self):
+    def _GetEpetraCommunicator(self):


I guess this is not intended but ok I would say

Ops. Search and replace... Should I keep it?

applications/FluidDynamicsApplication/python_scripts/fluid_solver.py

applications/FluidDynamicsApplication/python_scripts/navier_stokes_two_fluids_solver.py

philbucher · 2020-03-27T12:14:52Z

...cations/FluidDynamicsApplication/python_scripts/steady_navier_stokes_solver_vmsmonolithic.py

@@ -1,87 +0,0 @@
-from __future__ import print_function, absolute_import, division  # makes KratosMultiphysics backward compatible with python 2.6 and 2.7


this is no longer needed?

Removed from all solvers.

applications/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_embedded_solver.py

philbucher · 2020-03-27T12:17:59Z

applications/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_embedded_solver.py

+
+    @classmethod
+    def _FmAleIsActive(self):


this cannot be a classmethod!

mmm... Otherwise a warning is thrown. As there is no call to self I thought we need the @classmethod specifier.

well there is no necessity yet, but there will be (see baseclass)
btw why is this even special in Trilinos?

rubenzorrilla · 2020-03-27T13:13:02Z

Very nice work!
I think overall it goes in a good direction but I think there are still some flaws in the design, esp in how the settings are handled:
* The baseclass has no settings yet it uses them! This makes it very hard to follow where what is defined

(I already answered in your in-code comment)

* The trilinos solvers seem to duplicate pretty much all default settings from the (serial-) baseclass => I think this should be improved, check the structural solvers, I think we solved it in a good way

I don't agree at all... Actually our design avoids duplicating things from the base serial class (only the strategies, schemes, B&S are reimplemented). By doing this, we avoid reimplementing the rest methods of the solver. However, I agree that this comes at the price of having way duplication between the MPI solvers (embedded, monolithic, ...) as there is no intermediate base class (trilinos_fluid_solver.py) as you did in the structural solvers.

Long story short, I think that multiple inheritance would be the solution in here (if you feel brave enough you can proceed... 😄)

* Consider refactoring some methods to an external utility, e.g. like [this](https://github.com/KratosMultiphysics/Kratos/blob/master/kratos/python_scripts/auxiliary_solver_utilities.py) or [this](https://github.com/KratosMultiphysics/Kratos/blob/master/applications/StructuralMechanicsApplication/python_scripts/auxiliar_methods_adaptative_solvers.py). This should clean up the baseclass a lot

I'd left this for the future. At this point I'm trying to clean up the python_scripts folder of the application, which is IMO a complete mess.

Unfortunately there are also quite some cosmetic changes, e.g. in the logger or in the naming of existing functions. It would be very helpful from a review point of view if you could cherry-pick those if possible to a separate PR that goes in before this one

(I already answered in the in-code comments)

philbucher · 2020-03-27T16:01:07Z

I don't agree at all... Actually our design avoids duplicating things from the base serial class (only the strategies, schemes, B&S are reimplemented). By doing this, we avoid reimplementing the rest methods of the solver. However, I agree that this comes at the price of having way duplication between the MPI solvers (embedded, monolithic, ...) as there is no intermediate base class (trilinos_fluid_solver.py) as you did in the structural solvers.

I think you misunderstood, I meant the solver_settings
E.g. I just checked the monolithic solvers: the serial (base) class defines a bunch of settings, and the same setting are basically unchanged copy-pasted in the trilinos version

Long story short, I think that multiple inheritance would be the solution in here (if you feel brave enough you can proceed... smile)

I attempted this multiple times for this purpose but I could just not figure out how the MRO (method resolution order) works :(

rubenzorrilla · 2020-03-27T16:46:48Z

I don't agree at all... Actually our design avoids duplicating things from the base serial class (only the strategies, schemes, B&S are reimplemented). By doing this, we avoid reimplementing the rest methods of the solver. However, I agree that this comes at the price of having way duplication between the MPI solvers (embedded, monolithic, ...) as there is no intermediate base class (trilinos_fluid_solver.py) as you did in the structural solvers.

I think you misunderstood, I meant the solver_settings
E.g. I just checked the monolithic solvers: the serial (base) class defines a bunch of settings, and the same setting are basically unchanged copy-pasted in the trilinos version

Yes, I misunderstood your point. Totally agree about this. I'd do it in a further PRs

Long story short, I think that multiple inheritance would be the solution in here (if you feel brave enough you can proceed... smile)

I attempted this multiple times for this purpose but I could just not figure out how the MRO (method resolution order) works :(

philbucher · 2020-03-27T16:49:54Z

Yes, I misunderstood your point. Totally agree about this. I'd do it in a further PRs

ok makes sense to me

let me take a final look over the weekend and test it myself, then I will approve

rubenzorrilla · 2020-03-27T17:19:46Z

Yes, I misunderstood your point. Totally agree about this. I'd do it in a further PRs

ok makes sense to me

let me take a final look over the weekend and test it myself, then I will approve

I've already pushed the changes with your suggestions (let's see if I did not mess something).

Many thanks again.

philbucher

I checked this again, I pointed out some things for further improvements (please in a different PR)
It is still a large PR, I hope I didn't miss anything
Others please also check this carefully

regarding the "fake" scheme: Why don't you implement this properly, i.e. a scheme that only does the update? This way you would be safe in the sense that if the Scheme you are using right now changes then you wouldn't be affected

I also tried running the MPI-tests, but I get the following error :/ Not sure, probably my installation (although it hasn't changed in a long time)

  File "/home/philipp/software/Kratos_MasterBranch/install/KratosMultiphysics/FluidDynamicsApplication/trilinos_navier_stokes_solver_vmsmonolithic.py", line 8, in <module>
    from KratosMultiphysics.FluidDynamicsApplication import TrilinosExtension as TrilinosFluid
  File "/home/philipp/software/Kratos_MasterBranch/install/KratosMultiphysics/FluidDynamicsApplication/TrilinosExtension.py", line 4, in <module>
    from KratosFluidDynamicsTrilinosExtension import *
ImportError: /home/philipp/software/Kratos_MasterBranch/install/libs/KratosFluidDynamicsTrilinosExtension.cpython-36m-x86_64-linux-gnu.so: undefined symbol: _ZTI14Epetra_MpiComm

philbucher · 2020-03-29T23:14:33Z

...ions/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_solver_fractionalstep.py

        self.compute_reactions = self.settings["compute_reactions"].GetBool()

-        KratosMultiphysics.Logger.PrintInfo("TrilinosNavierStokesSolverFractionalStep","Construction of TrilinosNavierStokesSolverFractionalStep solver finished.")
+        self.main_model_part.ProcessInfo.SetValue(KratosMultiphysics.OSS_SWITCH, self.settings["oss_switch"].GetInt())
+        self.main_model_part.ProcessInfo.SetValue(KratosMultiphysics.DYNAMIC_TAU, self.settings["dynamic_tau"].GetDouble())


shouldn't those things also be done in the baseclass-constructor?

EDIT: I checked the implementation, it seems like some minor refactoring needs to be done, bcs right now the Trilinos-solver calls the parent of the parent 😅

but please in a future PR ;)

Agree.This was a workaround we did in the past (I guess because the defaults did not completely match). I already added a note in the Fluid Dynamics Application Project.

philbucher · 2020-03-29T23:17:30Z

...ions/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_solver_fractionalstep.py

+        velocity_linear_solver_configuration = self.settings["velocity_linear_solver_settings"]
+        velocity_linear_solver = trilinos_linear_solver_factory.ConstructSolver(velocity_linear_solver_configuration)
+        # Return a tuple containing both linear solvers
+        return (pressure_linear_solver, velocity_linear_solver)


nice solution 👍

philbucher · 2020-03-29T23:19:10Z

...tions/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_solver_vmsmonolithic.py

-            KratosMultiphysics.Logger.PrintInfo("TrilinosNavierStokesSolverMonolithic", "Using " + self.condition_name + " as wall condition")
-
-        KratosMultiphysics.Logger.Print("Construction of TrilinosNavierStokesSolverMonolithic finished.")
+            KratosMultiphysics.Logger.PrintInfo(self.__class__.__name__, "Using " + self.condition_name + " as wall condition")


same comment as for the FractionalStepSolver, I think with minor refactoring you could save a lot of code duplication when you call the baseclass constructor directly

Definitively (left for future PR)

philbucher · 2020-03-29T23:22:15Z

applications/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_embedded_solver.py

+
+    @classmethod
+    def _FmAleIsActive(self):


well there is no necessity yet, but there will be (see baseclass)
btw why is this even special in Trilinos?

philbucher · 2020-03-29T23:22:52Z

applications/FluidDynamicsApplication/python_scripts/trilinos_navier_stokes_embedded_solver.py

@@ -87,29 +78,24 @@ def GetDefaultSettings(cls):
    def __init__(self, model, custom_settings):
        self._validate_settings_in_baseclass=True # To be removed eventually
        # Note: deliberately calling the constructor of the base python solver (the parent of my parent)


I think with little refactoring this hack is no longer needed-

philbucher · 2020-03-29T23:27:53Z

applications/CompressiblePotentialFlowApplication/python_scripts/potential_flow_solver.py


-        return strategy
+    @classmethod


Suggested change

@classmethod

I think adding this is wrong, but I am not entirely sure
I mean, in the baseclass this is not a classmethod

The point is that this method could be a function, as it does not require the self. This turns into a Python warning.
If I'm not wrong the purpose of the @classmethod decorator is to explicitly indicate that the function is a method.

This turns into a Python warning.

well we ignore this often enough ;)

if you are sure this works/is ok then please go ahead

philbucher · 2020-03-29T23:28:17Z

applications/CompressiblePotentialFlowApplication/python_scripts/potential_flow_solver.py


    def AdvanceInTime(self, current_time):
        raise Exception("AdvanceInTime is not implemented. Potential Flow simulations are steady state.")

-    def _ComputeNodalNeighbours(self):
+    def ComputeNodalElementalNeighbours(self):


Suggested change

def ComputeNodalElementalNeighbours(self):

def _ComputeNodalElementalNeighbours(self):

philbucher · 2020-03-29T23:28:33Z

...cations/CompressiblePotentialFlowApplication/python_scripts/potential_flow_adjoint_solver.py

+            raise Exception("Invalid response_type: " + self.response_function_settings["response_type"].GetString())
+        return response_function
+
+    def GetSensitivityBuilder(self):


Suggested change

def GetSensitivityBuilder(self):

def _GetSensitivityBuilder(self):

philbucher · 2020-03-29T23:29:41Z

...cations/CompressiblePotentialFlowApplication/python_scripts/potential_flow_adjoint_solver.py

+        scheme = KratosMultiphysics.ResidualBasedAdjointStaticScheme(response_function)
+        return scheme
+
+    def GetResponseFunction(self):


Suggested change

def GetResponseFunction(self):

def _GetResponseFunction(self):

philbucher · 2020-03-29T23:30:22Z

applications/ChimeraApplication/python_scripts/navier_stokes_solver_vmsmonolithic_chimera.py

@@ -57,6 +57,9 @@ def Initialize(self):
            self.model,
            self.chimera_internal_parts)

+    def GetComputingModelPart(self):
+        return self.main_model_part


I discussed this many times with @jcotela, he said it is not possible to remove it
Not sure if this has changed

rubenzorrilla · 2020-03-30T07:48:37Z

I checked this again, I pointed out some things for further improvements (please in a different PR)
It is still a large PR, I hope I didn't miss anything
Others please also check this carefully
* regarding the "fake" scheme: Why don't you implement this properly, i.e. a scheme that only does the update? This way you would be safe in the sense that if the Scheme you are using right now changes then you wouldn't be affected

It is already implemented. We are using the ResidualBasedIncrementalUpdateStaticScheme. This only calls the initializations, the eqs. Ids. and does the update.

I also tried running the MPI-tests, but I get the following error :/ Not sure, probably my installation (although it hasn't changed in a long time)

  File "/home/philipp/software/Kratos_MasterBranch/install/KratosMultiphysics/FluidDynamicsApplication/trilinos_navier_stokes_solver_vmsmonolithic.py", line 8, in <module>
    from KratosMultiphysics.FluidDynamicsApplication import TrilinosExtension as TrilinosFluid
  File "/home/philipp/software/Kratos_MasterBranch/install/KratosMultiphysics/FluidDynamicsApplication/TrilinosExtension.py", line 4, in <module>
    from KratosFluidDynamicsTrilinosExtension import *
ImportError: /home/philipp/software/Kratos_MasterBranch/install/libs/KratosFluidDynamicsTrilinosExtension.cpython-36m-x86_64-linux-gnu.so: undefined symbol: _ZTI14Epetra_MpiComm

I seems it is... In any case I'll try again.

rubenzorrilla · 2020-03-30T08:02:12Z

I discussed this many times with @jcotela, he said it is not possible to remove it
Not sure if this has changed

I asked @adityaghantasala and he told me it is because the computing model part does not include the constraints.
I think we should improve this so he can also use it as the rest of CFD solvers.

(I don't know why I can't reply in line)

…atosMultiphysics/Kratos into fluid/fine-grained-initialize

philbucher · 2020-03-30T08:08:15Z

It is already implemented. We are using the ResidualBasedIncrementalUpdateStaticScheme. This only calls the initializations, the eqs. Ids. and does the update.

but in the FluidSolver you use ResidualBasedIncrementalUpdateStaticSchemeSlip

rubenzorrilla · 2020-03-30T13:05:00Z

It is already implemented. We are using the ResidualBasedIncrementalUpdateStaticScheme. This only calls the initializations, the eqs. Ids. and does the update.

but in the FluidSolver you use ResidualBasedIncrementalUpdateStaticSchemeSlip

Well, it's a derived class that also does the rotation for the slip BC. But essentially is the same.

philbucher · 2020-03-30T13:30:36Z

no further comments

Nice work 👍

I leave it to @KratosMultiphysics/fluid-dynamics to approve :)

rubenzorrilla · 2020-03-30T13:34:14Z

@adityaghantasala, @sunethwarna, @marcnunezc, @inigolcanalejo can you please re-approve?

rubenzorrilla · 2020-03-31T09:07:25Z

@adityaghantasala, @sunethwarna, @marcnunezc, @inigolcanalejo can you please re-approve?

@KratosMultiphysics/fluid-dynamics can someone re-approve? (this PR is quite sensible and conflict prone so I'd like to merge it ASAP).

marcnunezc

Ok from our side!

RiccardoRossi

ok

rubenzorrilla added 15 commits March 14, 2020 16:26

Base solver

96e5fb4

Working monolithic and FS solvers

e2ab850

Serial solvers working

d5035d1

Remove old steady solver

ac17ab4

Serial solvers clean up

2caacb2

MPI solvers (testing is pending)

889a4f2

Merge branch 'master' into fluid/fine-grained-initialize

aec2a26

Adding missing Trilinos scheme

17159d8

Fix name mangling

b98d832

Minor

3014ff8

Using AMGCL as default linear solver

e06ca98

Using self.__class__.__name__

7ff8119

Adjoint solvers

11fc9ab

Chimera solvers

aebc95d

Minor correction

bd58e39

rubenzorrilla added Help Wanted Cleanup Applications Python Convention Usability labels Mar 20, 2020

rubenzorrilla added this to the Release 8.0 milestone Mar 20, 2020

rubenzorrilla requested review from adityaghantasala, sunethwarna and philbucher March 20, 2020 01:26

rubenzorrilla self-assigned this Mar 20, 2020

rubenzorrilla changed the title ~~[Fluid] Fine grained Pyrhon solvers~~ [Fluid] Fine grained Python solvers Mar 20, 2020

rubenzorrilla mentioned this pull request Mar 23, 2020

Errors in tests #6431

Closed

philbucher reviewed Mar 27, 2020

View reviewed changes

Merge branch 'master' into fluid/fine-grained-initialize

d59aeeb

@philbucher suggestions

b7b6858

rubenzorrilla and others added 3 commits March 27, 2020 18:29

Missing aux Darcy test solver change

d758736

Merge branch 'master' into fluid/fine-grained-initialize

6213eee

fixing NavierStokesAleFluidSolver

339612d

philbucher reviewed Mar 29, 2020

View reviewed changes

rubenzorrilla added 2 commits March 30, 2020 10:02

Potential flow style guide

2f7b4ca

Merge branch 'fluid/fine-grained-initialize' of https://github.com/Kr…

f65a3ae

…atosMultiphysics/Kratos into fluid/fine-grained-initialize

marcnunezc approved these changes Mar 31, 2020

View reviewed changes

RiccardoRossi approved these changes Mar 31, 2020

View reviewed changes

rubenzorrilla merged commit d578593 into master Mar 31, 2020

rubenzorrilla deleted the fluid/fine-grained-initialize branch March 31, 2020 09:26

		@@ -1,87 +0,0 @@
		from __future__ import print_function, absolute_import, division # makes KratosMultiphysics backward compatible with python 2.6 and 2.7

	def ComputeNodalElementalNeighbours(self):
	def _ComputeNodalElementalNeighbours(self):

	def GetSensitivityBuilder(self):
	def _GetSensitivityBuilder(self):

	def GetResponseFunction(self):
	def _GetResponseFunction(self):

[Fluid] Fine grained Python solvers #6575

[Fluid] Fine grained Python solvers #6575

Conversation

rubenzorrilla commented Mar 20, 2020 • edited Loading

rubenzorrilla commented Mar 20, 2020

adityaghantasala commented Mar 20, 2020

philbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenzorrilla commented Mar 27, 2020 • edited Loading

philbucher commented Mar 27, 2020

rubenzorrilla commented Mar 27, 2020

philbucher commented Mar 27, 2020

rubenzorrilla commented Mar 27, 2020

philbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenzorrilla commented Mar 30, 2020

rubenzorrilla commented Mar 30, 2020

philbucher commented Mar 30, 2020

rubenzorrilla commented Mar 30, 2020

philbucher commented Mar 30, 2020

rubenzorrilla commented Mar 30, 2020

rubenzorrilla commented Mar 31, 2020

marcnunezc left a comment

Choose a reason for hiding this comment

RiccardoRossi left a comment

Choose a reason for hiding this comment

rubenzorrilla commented Mar 20, 2020 •

edited

Loading

rubenzorrilla commented Mar 27, 2020 •

edited

Loading