DelNov / T-Flows

Program for Simulation of Turbulent Flows
Other
113 stars 50 forks source link

c2 < 0 #204

Closed palkinev closed 4 years ago

palkinev commented 4 years ago

Dear Bojan,

This pull request targets issue #200.

Var_Type now has new attribute: integer, allocatable :: bnd_cond_type(:)

which is 0 by default and in other case it matches BC unique index from Bnd_Cond_Mod.

For u, v, w, p, t, sc it can be accessed as flow % var % bnd_cond_type() (pp % bnd_cond_type is not defined) For all other turbulent vars turb % bnd_cond_type() is used.

Thus, in a large bunch of cases c2 < 0 could be omitted.

However there are some subroutines which are unreachable for Var_Type like Grid_Mod. Therefore for Convert and Divide I have not changed anything.

In some cases like Field_Mod_Grad_Component I feel like we should send bnd_cond_type as argument of integer type in order to improve overall performance on this function.

In other cases like in Update_Boundary_Values the code for heated area and heat must be reworked.

And I also refrained myself from editing Vtu and VoF related functions.

Niceno commented 4 years ago

Dear Egor,

I am not sure this is the direction in which we should go. My initial proposal was to (I cite) "if we were storing boundary condition information in faces, rather than in boundary cells. In your pull request we still store boundary condition information in boundary cells. In addition, things got even more complex in my opinion. Before we were checking boundary conditions by two member functions (interfaces): Grid_Mod_Bnd_Cond_Type(grid, c2) and Var_Mod_Bnd_Cond_Type(var, c2) which was already ambiguous and a bit confusing, but now we have the third option: turb % bnd_cond_type(c2). We are also replacing access to data member (array bnd_cond_type) via interface (functions mentioned above) with direct access to data member which reduces code flexibility. I am also a bit afraid that this pull request would impact people working in other fields, particle tracking and volume of fluid. I am not sure, I would leave this issue open for future and go in evolutionary steps. For example, we may first want to try to eliminate Grid_Mod_Bnd_Cond_Type and rely on Var_Mod_Bnd_Cond_Type only - if possible. Once this is achieved, we could define boundary conditions at the faces in addition to boundary cells, and gradually eliminating checks if(c2 < 0) - if and where possible. Something like that. I am afraid that c2 < 0 checks are so deeply embedded in the code that we can't get rid of them in a single stroke.

palkinev commented 4 years ago

Ok, Bojan, perhaps I went too far on enhancing code appearance in all of subroutines, which contained c2 < 0 lines. But I think that it won't take much effort to substitute bnd_cond_type(c2) to bnd_cond_type(s) if it is redefined in Initialize_Variables.f90. However this doesn't reduce ambiguity for turbulent variables and I agree that one should start from Var_Mod_Bnd_Cond_Type.

Niceno commented 4 years ago

It is too early for this and we had many other developments in the meanwhile which conflict with this, I will therefore close it.