[Core] Class "ParallelDistanceCalculator" requires deprecated variable "IS_VISITED"

[swenczowski]

Several function members of the class ParallelDistanceCalculator (in "kratos/utilities/parallel_levelset_distance_calculator.h") such as

• CalculateDistancesLagrangianSurface()
• AddDistanceToNodesNew()
• AddDistanceToNodes()
• and further private functions

are hard-coded to operate on the variable IS_VISITED(in most cases a non-historical variable). Though still widely used, this variable of type double seems to be deprecated and can only be used by including the header "includes/deprecated_variables.h". Within the code of class ParallelDistanceCalculator, the variable is only assigned the values 0.0 or 1.0

If I understood @RiccardoRossi correctly, it would be advantageous in concern of memory to use the flag VISITED instead of the variable IS_VISITED.

If you do not see any issues or expect obstacles in the implementation, I would try and adapt the definition of the class accordingly. In this case, should the flag be passed as an argument or would you prefer a hard-coded version as it is currently the case with IS_VISITED?

[rubenzorrilla]

This would be quite positive for all of us so go ahead.

My unique concern is that the flag VISITED might be used by another utilities. So we can take the chance to make it safer by creating a local flag to do this (check how the macro KRATOS_CREATE_LOCAL_FLAG is used). Honestly, I don't know if these local flag have some overhead over the common ones. @jcotela , do you know about this?

[philbucher]

isn't the issue with flags the same one as in #214 ?

[rubenzorrilla]

Yes, indeed I've just realised that we cannot implement this until #1277 is finished.

[adityaghantasala]

We are using IS_VISITED in ChimeraApplication. So I will prefer that a local flag is used ! Eventually we will also define and use a local flag in ChimeraApplicaiton.

[swenczowski]

@rubenzorrilla I am sorry for interfering. Unfortunately, I am not sure if a local flag is applicable, here. The value of the variable must be set from outside the function to describe an initial state specifying which nodes the function should start from. A typical case (in bad psuedo-code) would be:

for node in ModelPartA: set IS_VISITED = 0.0 for node in ModelPartB: set IS_VISITED = 1.0 call ParallelDistanceCalculator.Function()

[jcotela]

Just to confirm the statements of @rubenzorrilla and @philbucher: we do not have the capability to combine flag values from different processes at the moment, which limits us to the less efficient double variables you are using now. I have a branch implementing this feature (see #3672), but it is unfortunately not ready for merge at the moment.

[philbucher]

@rubenzorrilla I am sorry for interfering. Unfortunately, I am not sure if a local flag is applicable, here. The value of the variable must be set from outside the function to describe an initial state specifying which nodes the function should start from. A typical case (in bad psuedo-code) would be:

for node in ModelPartA: set IS_VISITED = 0.0 for node in ModelPartB: set IS_VISITED = 1.0 call ParallelDistanceCalculator.Function()

I think you could also specify local-flags from outside if you expose them Not sure though/didn't find an example

then it would be sth like Set(ParallelDistanceCalculator.IS_VISITED_LOCAL_FLAG)

[RiccardoRossi]

I think that all comments here are relevant. As a comment to @adityaghantasala i would say that it is not a good practice to assume VISITED as a "permanent" value, since this flag is very often used for things that needs to swipe once through the mesh.

[philbucher]

Is this solved?

[jcotela]

Did we reach a conclusion? Now flags can be synchronized in MPI just like nodal double variables, so at least this technical limitation is no longer there.

[rubenzorrilla]

@RiccardoRossi I've checked where the ParallelDistanceCalculator is used. It is only used in the deprecated incompressible_fluid_application and in the edge based solvers in the FreeSurfaceApplication.

At first glance, it seems to me that this utility has a similar behavior to the VariationalDistanceProcess, which is to say, to compute de distance field provided the zero values of the level-set function but in this case without solving a variational problem. Please correct me if I'm wrong.

Having said this? I'm wondering if it makes sense to fix the issues raised above or to directly deprecate the utility. I think you implemented this utility so you could better clarify which are the main features of it and if it makes sense to maintain it.

[RiccardoRossi]

i would not remove this. it can be used from python and it works good. in a sense it may also be more reliabke than the variational one

On Sat, Sep 21, 2019, 2:19 PM Rubén Zorrilla notifications@github.com wrote:

@RiccardoRossi https://github.com/RiccardoRossi I've checked where the ParallelDistanceCalculator is used. It is only used in the deprecated incompressible_fluid_application and in the edge based solvers in the FreeSurfaceApplication.

At first glance, it seems to me that this utility has a similar behavior to the VariationalDistanceProcess, which is to say, to compute de distance field provided the zero values of the level-set function but in this case without solving a variational problem. Please correct me if I'm wrong.

Having said this? I'm wondering if it makes sense to fix the issues raised above or to directly deprecate the utility. I think you implemented this utility so you could better clarify which are the main features of it and if it makes sense to maintain it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KratosMultiphysics/Kratos/issues/3882?email_source=notifications&email_token=AB5PWELUGTYBXYJDA5OXXC3QKYGNVA5CNFSM4GQ3QQC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IQXII#issuecomment-533793697, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5PWEIAJHASAM5S3EBFQSDQKYGNVANCNFSM4GQ3QQCQ .

[RiccardoRossi]

nevertheless i am.completely ok with using the flag instead of the variable if possible

On Sat, Sep 21, 2019, 8:38 PM Riccardo Rossi rrossi@cimne.upc.edu wrote:

i would not remove this. it can be used from python and it works good. in a sense it may also be more reliabke than the variational one

On Sat, Sep 21, 2019, 2:19 PM Rubén Zorrilla notifications@github.com wrote:

@RiccardoRossi https://github.com/RiccardoRossi I've checked where the ParallelDistanceCalculator is used. It is only used in the deprecated incompressible_fluid_application and in the edge based solvers in the FreeSurfaceApplication.

At first glance, it seems to me that this utility has a similar behavior to the VariationalDistanceProcess, which is to say, to compute de distance field provided the zero values of the level-set function but in this case without solving a variational problem. Please correct me if I'm wrong.

Having said this? I'm wondering if it makes sense to fix the issues raised above or to directly deprecate the utility. I think you implemented this utility so you could better clarify which are the main features of it and if it makes sense to maintain it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KratosMultiphysics/Kratos/issues/3882?email_source=notifications&email_token=AB5PWELUGTYBXYJDA5OXXC3QKYGNVA5CNFSM4GQ3QQC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7IQXII#issuecomment-533793697, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5PWEIAJHASAM5S3EBFQSDQKYGNVANCNFSM4GQ3QQCQ .

[ddiezrod]

We use this utility in our side. As Riccardo said its more "stable" in some cases and also allows us to save some memory. +1 to use the flag.

[rubenzorrilla]

We use this utility in our side. As Riccardo said its more "stable" in some cases and also allows us to save some memory. +1 to use the flag.

@ddiezrod I've never used this one, so just to satisfy my curiosity, when you say "more stable in some cases", which are these cases?

Thanks in advance.

(BTW, I'll try to take care of the flag issue ASAP).

[ddiezrod]

@rubenzorrilla For example, if you have an U shape where fluid advances from both ends the elements in the middle are problematic as they try to have a gradient = 1 but they do not know in which direction. @pablobecker spent some time checking these problems, so maybe he can add something more.

ParallelExtrapolationUtility is more robust and consumes less memory. We only care for distances near the free surface so thats why we use it in casting.

[pablobecker]

both parallel and variational one have issues: as @ddiezrod pointed out, the issue appears when relatively complex geometries have front meetings:

In the variational one, each element "tries" to get a distance gradient of 1, including elements where the fronts will meet. This region should have a gradient of 0 actually. This causes issues in some cases. The variational redistance might even blow up (recall the the variational one is not fully implicit)

In the parallel one, if elements have different sizes on the two different fronts, then distance at the "weld" (where the process computes the furthest distance before meeting the distance computed using the other front) will have a jump. If this jump is too large and relatively close to zero, distance can go negative when you convect it, creating artificial water droplets ( distance convection elements do not have shock capturing terms)

[rubenzorrilla]

I think I got your point. Thanks guys for the explanation!

[philbucher]

is this solved?

[rubenzorrilla]

is this solved?

Not yet.