[FluidApp] Adding CFD utilities to calculate Y_PLUS and U_TAU #7039

sunethwarna · 2020-06-09T10:44:29Z

This adds a "CFDUtilities" namespace which includes following methods

Linear-Logarithmic y_plus limit calculation method based on kappa and beta
y_plus and u_tau calculation based on linear-logarithmic law for slip walls (velocity is taken from condition center and tangential velocity is calculated). In here wall distance is wall normal distance to the center of the parent element of each condition. y_plus and u_tau is stored in condition data value container
y_plus and u_tau calculation based on tangential reaction. (no wall laws assumed). y_plus and u_tau is stored in condition data value container
A method to distribute y_plus and u_tau to nodes from conditions

A seperate process with json interface is also added to this PR, if someone prefers to calculate y_plus and u_tau without altering run scripts. Otherwise, all CFDUtility methods are exposed to python level so one can use directly those methods. Tests are also added. FluidDynamics tests are passing in FullDebug.

Require #7113 for windows compilation

…plus

RiccardoRossi

sparse comments, although i am not done with the revision

RiccardoRossi · 2020-06-09T16:22:08Z

applications/FluidDynamicsApplication/custom_python/add_custom_utilities_to_python.cpp

+    m.def_submodule("CFDUtilities")
+        .def("CalculateNumberOfNeighbourConditions", &CFDUtilities::CalculateNumberOfNeighbourConditions)
+        .def("CalculateLinearLogarithmicWallFunctionBasedYPlusLimit", &CFDUtilities::CalculateLinearLogarithmicWallFunctionBasedYPlusLimit,
+            py::arg("von_karman") = 0.41,


this is done this way to oblige using kwargs?

I did this merely to avoid everytime passing the well known values for constants used in wall modelling.

to the best pf my understanding (it these are kwargs) you are impicitly adding a dictionary on every call.

this will be negligible if you call once the function for many elements, however it wouldn't be acceptable if you are calling it once per node.

These python level calls are not used at the moment, I just exposed them to python in case someone wants to use them in their own scripts. If someone uses them, they need to call this method for every entity (node, element, condition). So I will remove them.

ping @sunethwarna

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

RiccardoRossi · 2020-06-09T16:25:01Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

@@ -0,0 +1,442 @@
+//    |  /           |


I am a bit lost. You are adding a lot of unrelated stuff in here no?

There are few unrelated stuff like EvaluateInPoint, CalculateConditionGeometryData, which are copies taken from elements and conditions. I couldn't find a central place where they are implemented for reuse, each time we define them in the elements or conditions. :/

CalculateConditionNormal method is there in NormalCalculataionUtils, but it assigns nodal historical value NORMAL as well which is problematic. That is because if these methods are used along with wall boundaries, and there is an adjacent SLIP boundary, using the method in NormalCalculationUtils will overwrite NORMALs of common nodes to wall boundaries and SLIP boundaries with wall boundary normals. :/

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

RiccardoRossi · 2020-06-09T16:29:58Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+
+    const int number_of_conditions = rModelPart.NumberOfConditions();
+#pragma omp parallel for
+    for (int i_cond = 0; i_cond < number_of_conditions; ++i_cond)


I would advise using the loop as in the parallel utilities:

https://github.com/KratosMultiphysics/Kratos/wiki/Parallel-Utilities

that would make your code cleaner

done :) It is really clean now :) thanks...

RiccardoRossi · 2020-06-10T06:43:20Z

applications/FluidDynamicsApplication/custom_python/add_custom_utilities_to_python.cpp

+    m.def_submodule("CFDUtilities")
+        .def("CalculateNumberOfNeighbourConditions", &CFDUtilities::CalculateNumberOfNeighbourConditions)
+        .def("CalculateLinearLogarithmicWallFunctionBasedYPlusLimit", &CFDUtilities::CalculateLinearLogarithmicWallFunctionBasedYPlusLimit,
+            py::arg("von_karman") = 0.41,


to the best pf my understanding (it these are kwargs) you are impicitly adding a dictionary on every call.

this will be negligible if you call once the function for many elements, however it wouldn't be acceptable if you are calling it once per node.

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.h

RiccardoRossi · 2020-06-10T06:47:58Z

applications/FluidDynamicsApplication/python_scripts/cfd_function_process.py

+
+class CFDFunctions:
+    @staticmethod
+    def GetFunction(params):


what is a CFDFunction?

a process? I am a bit lost about the use of this, in particular about the role of "GetFunction"

GetFunction is not a process. It is a wrapper for cfd utility methods. These are wrapper functions to be used in the json settings. All of them are standalone functions. As an example, this way it enables me to use reaction based y+ (for no slip) and wall law based y+(for slip with wall functions) calculations in the same simulation, and distribute them to nodes appropriately. (Still the common node between no slip and slip will have a problem).

This also makes having new cfd methods easily extensible to json settings using the same cfd_function_process.py. No need to have different python processes for different cfd utility methods.

THB I don't love the idea of implementing an additional concept in parallel to the processes. I.e. this could easily be done with separate processes (for which several py-scripts are needed ofc).

Is the only reason to do it as you propose bcs you don't want the additional py-scripts? Or would you have additional overhead?

To me the current script (cfd_function_process.py) is completely unreadable :/

If we really want to go this way then why not what is done in the HDF or the statistics app? There it is cleaner IMO

RiccardoRossi · 2020-06-10T06:59:44Z

applications/FluidDynamicsApplication/tests/Cavity/cfd_function_process.json

+                "Parameters": {
+                    "model_part_name": "MainModelPart.NoSlip2D_left_wall",
+                    "function_settings": {
+                        "function_name": "CalculateYPlusAndUTauForConditionsBasedOnReaction",


why didn't u use two distinct processes for this and for the next?

RiccardoRossi · 2020-06-10T07:00:20Z

applications/FluidDynamicsApplication/tests/Cavity/cfd_function_process.json

+                "help": "",
+                "Parameters": {
+                    "model_part_name": "MainModelPart",
+                    "function_settings": {


same, theses should be separated processes...i see no reason for having a nested function_settings

The reason behind this is, if I am to use different processes, then I need to have different python files which will have same code redundancy. I wanted to reduce code redundancy as much as possible.

the point is that doing the way you do it is non-standard.

wouldn't it be possible to have a base_cfd_utility_process which implements the common code, and to derive all other processes from that one?

Thats possible. I will change it like that :)

THB I don't love the idea of implementing an additional concept in parallel to the processes. I.e. this could easily be done with separate processes (for which several py-scripts are needed ofc).

Is the only reason to do it as you propose bcs you don't want the additional py-scripts? Or would you have additional overhead?

To me the current script (cfd_function_process.py) is completely unreadable :/

If we really want to go this way then why not what is done in the HDF or the statistics app? There it is cleaner IMO

I understand your point, as you stated, only reason I did it like this is to avoid maintaining several py files, make it easier for extending for new methods easily, keep list of files in check inside python_scripts folder. I don't think there is an additional overhead if I do seperate py files, except for overhead in maintaining them.

Could I use folder sctructure in python_scripts with __init__.py? @philbucher @RiccardoRossi @rubenzorrilla

Could I use folder sctructure in python_scripts with init.py

yes, see the CoSimApp

we can speak in person tmr, I think this way we can eliminate some concerns

rubenzorrilla · 2020-06-10T08:00:37Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+
+namespace Kratos
+{
+namespace CFDUtilities


Why a separated namespace?

Just to make sure that there aren't any conflicting names. Do you think it is not necessary?

Well, I don't think it harms but we normally keep everything in the same namespace. If we want to start using them, I will consensus how to do so to be consistent among applications.

Any comment about the namespace issues?

Although it is ok for me to have everything under one namespace, I am in favor of using different namespaces :). In most of the utilities available in Kratos, we are used to have classes and public and/or static methods to nicely package them. I was thinking to do it using namespaces, so then these namespaces can be extended in different files to as it develops. But I have no clue on how to properly adhere to Kratos guide lines about use of namespaces :/ :|

…plus

rubenzorrilla · 2020-06-30T09:32:56Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+//  License:		 BSD License
+//					 Kratos default license: kratos/license.txt
+//
+//  Main authors:    Suneth Warnakulasuriya (https://github.com/sunethwarna)


Header format (tabs and link)

rubenzorrilla · 2020-06-30T09:35:10Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+    }
+}
+
+double CalculateConditionWallHeight(const ConditionType& rCondition, const array_1d<double, 3>& rNormal)


Suggested change

double CalculateConditionWallHeight(const ConditionType& rCondition, const array_1d<double, 3>& rNormal)

double CalculateConditionWallHeight(

const ConditionType& rCondition,

const array_1d<double, 3>& rNormal)

Style guide for more than one argument functions.

rubenzorrilla · 2020-06-30T09:39:20Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+    rNContainer = rGeometry.ShapeFunctionsValues(rIntegrationMethod);
+
+    // CAUTION: "Jacobian" is 2.0*A for triangles but 0.5*A for lines
+    double det_J = (dimension == 2) ? 0.5 * domain_size : 2.0 * domain_size;


Why don't we call the Jacobian of the geometry? Despite it is more expensive, it would make this compatible with any geometry.

rubenzorrilla · 2020-06-30T09:44:56Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+double CalculateLinearLogarithmicWallFunctionBasedYPlusLimit(
+    const double VonKarman, const double WallSmoothness, const int MaxIterations, const double Tolerance)


Suggested change

double CalculateLinearLogarithmicWallFunctionBasedYPlusLimit(

const double VonKarman, const double WallSmoothness, const int MaxIterations, const double Tolerance)

double CalculateLinearLogarithmicWallFunctionBasedYPlusLimit(

const double VonKarman,

const double WallSmoothness,

const int MaxIterations,

const double Tolerance)

rubenzorrilla · 2020-06-30T09:47:39Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+    const int MaxIterations,
+    const double Tolerance)


I'd give default arguments to these (same above)

rubenzorrilla · 2020-06-30T10:11:08Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+            const Vector& gauss_shape_functions = row(shape_functions, 0);
+
+            // calculate normal for the condition
+            const array_1d<double, 3> normal = r_geometry.Normal(local_point);


Suggested change

const array_1d<double, 3> normal = r_geometry.Normal(local_point);

const auto normal = r_geometry.Normal(local_point);

rubenzorrilla · 2020-06-30T10:15:04Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+
+void CalculateYPlusAndUTauForConditions(
+    ModelPart& rModelPart,
+    const Variable<double>& rKinematicViscosityVariable,


Note what we discussed about Kinematic vs. Dynamic viscosity. Some years ago we decided to favor the use of the dynamic viscosity to be consistent among the different elements. I suggest following this in here.

rubenzorrilla · 2020-06-30T10:15:15Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+    array_1d<double, 3>& rFrictionVelocity,
+    const array_1d<double, 3>& rWallVelocity,
+    const array_1d<double, 3>& rNormal,
+    const double KinematicViscosity,


See the viscosity comment below.

rubenzorrilla · 2020-06-30T10:15:46Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+
+void CalculateYPlusAndUTauForConditionsBasedOnReaction(
+    ModelPart& rModelPart,
+    const Variable<double>& rKinematicViscosityVariable,


Same viscosity comment.

rubenzorrilla · 2020-06-30T10:20:59Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.cpp

+    const int MaxIterations,
+    const double Tolerance)


I'd set defaults for these.

rubenzorrilla · 2020-06-30T10:22:10Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.h

+//  License:		 BSD License
+//					 Kratos default license: kratos/license.txt
+//
+//  Main authors:    Suneth Warnakulasuriya


Header tabs.

rubenzorrilla · 2020-06-30T10:26:34Z

applications/FluidDynamicsApplication/custom_utilities/cfd_utilities.h

+namespace CFDUtilities
+{


To follow what we normally do, I'd create a WallLawUtilities, WallUtilities, CFDWallUtilities or whatever name you prefer class with static methods to follow what we normally do. If there is an specific reason to implement these as functions in a new namespace we can discuss about it.

I disagree, to me using namespaces aka free functions is much cleaner
I.e. when using a class one never knows if e.g. something is saved internally or not etc
With free functions this is very clear
Plus it follows what the standard library does

I think using classes with static methods is a relic bcs we didn’t know a better way
Esp the py-exposure was a problem I think, but now it is solved

rubenzorrilla · 2020-06-30T10:32:20Z

applications/FluidDynamicsApplication/python_scripts/cfd_utility_function_process.py

+    return CFDUtilityFunctionProcess(model, settings["Parameters"])
+
+
+class CFDUtilityFunctionProcess(Kratos.Process):


I'm not sure about exposing these as a process to the user...

Instead, I think we can add the settings to the turbulence_model_solver_settings in the solver_settings. Then the methods must be automatically called internally.

rubenzorrilla · 2020-06-30T10:35:03Z

applications/FluidDynamicsApplication/python_scripts/cfd_utility_functions.py

@@ -0,0 +1,132 @@
+# Importing the Kratos Library


If the CFDUtilities are implemented as a class with static methods this could be avoided by exposing such class to Python.

Concerning the Parameters input, I think it should be implemented also in the C++ level. The ModelPart + Parameters version of the methods can call the "standard" functions that are already there.

rubenzorrilla

I think we still need to discuss about the namespace vs. class issue as well as the Python exposition.

philbucher · 2020-06-30T10:44:38Z

I think we still need to discuss about the namespace vs. class issue as well as the Python exposition.

I guess this is then for the @KratosMultiphysics/technical-committee

What are your reasons for using class?

I honestly thought that this was dropped long ago, I use namespaces for years already

rubenzorrilla · 2020-07-07T08:30:11Z

I think we still need to discuss about the namespace vs. class issue as well as the Python exposition.

I guess this is then for the @KratosMultiphysics/technical-committee

What are your reasons for using class?

I honestly thought that this was dropped long ago, I use namespaces for years already

Well, I've no specific reason more than historical reasons and code organization. I have the feeling that it's easier to search for things if you now they are stored in one class rather than open the possibility to add things to a namespace from different files (possibly this is because I'm not used to work with namespaces).

RiccardoRossi · 2020-09-08T08:39:22Z

with respect to the classes vs namespace issue please refer to #7434

sunethwarna · 2022-08-17T14:14:56Z

closing since this is done in #10097 .

sunethwarna added 10 commits June 4, 2020 16:56

adding variables

5b982fe

initial cfd_utilities commit

8fb798e

adding cfd utilities

565c494

adding cfd_function_process.py

b627bf1

adding cfd_utility tests

aeaeae7

bug fix

acf2a78

adding cfd_function_process minor change

f4254cb

adding cfd_function_process test

a7779bd

method name change

ab096c3

add test to Fluid app

a7e29ed

sunethwarna added Applications Python C++ labels Jun 9, 2020

sunethwarna requested review from mpentek, rubenzorrilla and philbucher June 9, 2020 10:44

sunethwarna self-assigned this Jun 9, 2020

sunethwarna added 8 commits June 9, 2020 12:59

bug fix

af809dc

remove unused variable

55aecd7

remove unused import

76b097f

merge remote-tracking branch 'origin/master' into flud-cfd-utility-y-…

e8be43b

…plus

update

b9613a3

adding KRATOS_API

65408f0

using isinstance

2506440

fix KRATOS_API

23393f1

RiccardoRossi reviewed Jun 9, 2020

View reviewed changes

sunethwarna added 2 commits June 10, 2020 00:55

using parallel_utilities.h

b47bd33

adding doxygen comments

4e0bf6d

RiccardoRossi reviewed Jun 10, 2020

View reviewed changes

rubenzorrilla reviewed Jun 10, 2020

View reviewed changes

sunethwarna added 2 commits June 27, 2020 07:47

fix variable type conflict

e0c065d

applying codacy suggestions

9e4317c

sunethwarna requested a review from rubenzorrilla June 27, 2020 10:11

sunethwarna mentioned this pull request Jun 27, 2020

[Core] Add distribute variable method to variable utils #7146

Merged

sunethwarna added 2 commits June 28, 2020 09:16

Merge remote-tracking branch 'origin/master' into flud-cfd-utility-y-…

809efc3

…plus

adopt to newer variable_utils

bc7c234

rubenzorrilla reviewed Jun 30, 2020

View reviewed changes

rubenzorrilla requested changes Jun 30, 2020

View reviewed changes

RiccardoRossi mentioned this pull request Sep 8, 2020

Classes VS Namespaces #7434

Closed

roigcarlo linked an issue Sep 10, 2020 that may be closed by this pull request

Classes VS Namespaces #7434

Closed

roigcarlo removed a link to an issue Sep 10, 2020

Classes VS Namespaces #7434

Closed

sunethwarna closed this Aug 17, 2022

sunethwarna deleted the flud-cfd-utility-y-plus branch August 17, 2022 14:15

		double CalculateLinearLogarithmicWallFunctionBasedYPlusLimit(
		const double VonKarman, const double WallSmoothness, const int MaxIterations, const double Tolerance)

	const array_1d<double, 3> normal = r_geometry.Normal(local_point);
	const auto normal = r_geometry.Normal(local_point);

		return CFDUtilityFunctionProcess(model, settings["Parameters"])


		class CFDUtilityFunctionProcess(Kratos.Process):

[FluidApp] Adding CFD utilities to calculate Y_PLUS and U_TAU #7039

[FluidApp] Adding CFD utilities to calculate Y_PLUS and U_TAU #7039

Conversation

sunethwarna commented Jun 9, 2020 • edited Loading

RiccardoRossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunethwarna Jun 9, 2020 • edited Loading

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

sunethwarna Jun 9, 2020 • edited Loading

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

sunethwarna Jun 15, 2020 • edited Loading

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 Jun 30, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

philbucher commented Jun 30, 2020

rubenzorrilla commented Jul 7, 2020

RiccardoRossi commented Sep 8, 2020

sunethwarna commented Aug 17, 2022

sunethwarna commented Jun 9, 2020 •

edited

Loading

sunethwarna Jun 9, 2020 •

edited

Loading

sunethwarna Jun 9, 2020 •

edited

Loading

sunethwarna Jun 15, 2020 •

edited

Loading

rubenzorrilla Jun 30, 2020 •

edited

Loading