Kratos: MPI flags synchronize

[rubenzorrilla]

After a short discussion with @jcotela we realized that still not have any convention about the flag synchronization in MPI. As a preliminary idea, we think that we could add the next two methods:

• SynchronizeFlagAnd()
• SynchronizeFlagOr()

that perform the and and or boolean operations respectively.

Any suggestion/idea about that?

[philbucher]

Do you mean Kratos::Flags? Could you give an example of where you use it?

[rubenzorrilla]

I would like to use it for tagging the inner nodes that shouldn't be fixed in embedded formulations that have this requirement. For the moment we are using an integer variable and it would be nice to get rid of that.

[pooyan-dadvand]

An open question, should we consider the IsDefined property of the flags also?

[pooyan-dadvand]

This opens another issue that Flags does the same for & and | !!! yes, this is the result of having the arguments passing to functions like:

foo(ACTIVE & SELECTED) 

to be understand as applying both of them instead of using the or:

foo(ACTIVE | SELECTED) 

I remember that @RiccardoRossi was in favor of this behavior but IMO is misleading using the Flags.

To be discussed in @KratosMultiphysics/technical-committee but any comment is welcomed!

[pooyan-dadvand]

The conclusion of TC discussion is to put the & deprecated to find out the use of it and put all those usage with second (union) syntax.
The the synchronize commands will consider only the set flags in voting for and/or

[RiccardoRossi]

@KratosMultiphysics/technical-committee has this been done? if so let's close this

[RiccardoRossi]

If one mpi node does not define, it does not participate in the decision

if no one has it defined it is left unset

[jcotela]

@KratosMultiphysics/technical-committee Shall we go ahead with this? I'm temptatively assigning the After Release label.

[philbucher]

This is waiting for the MPI-Core right?

[jcotela]

@philbucher yes, it requires both #3340 and #3347.

[jcotela]

In preparation for this PR, I tried to document the expected results of flag operations in a set of tests. For flags where only one of the two members is set, I followed the convention of "flags that are not set do not participate in the operation" as decided above, which results in the following "extended" truth tables:
https://github.com/KratosMultiphysics/Kratos/blob/22cf43f876a17c65ee5e754e636ff282dbdef1b3/kratos/tests/test_flags.py#L96-L142

Do we all agree that this is the desired behavior? I would prefer not to put this into production code unless we have a consensus. @KratosMultiphysics/design-committee @ddiezrod.

[jcotela]

Curiosity: the proposed approach implies that node.Set(ACTIVE & ERASE) sets BOTH flags, as @RiccardoRossi originally intended.

[pooyan-dadvand]

No.

It just set NOT_ACTIVE and NOT_ERASE because they are in different bits and doing and with them make all valul flags 0 and set is_defined to 1 in corresponding bits

Just to see it more visual:

Flag          ACTIVE  &  ERASE  
Value       : 000010  & 001000  =  000000
Is defined  : 000010  | 001000  =  001010

You should use node.Set(ACTIVE | ERASE) to set both (like in C) which is not very intuitive for ones not familiar with C.

[jcotela]

@pooyan-dadvand I agree with you in principle, but the behavior we are imposing for communication would imply that: (I am using ACTIVE and ERASE to facilitate discussion, but you can also assume that they are the same flag in two different processes)

Flag       ACTIVE
Value      000010
Is Defined 000010

Flag       ERASE
Value      001000
Is Defined 001000

Flag       : ACTIVE & ERASE
Is Defined : Defined(ACTIVE) | Defined(ERASE) : 001010
Value      : "Only defined values participate in the communication" 
             For &, this implies undefined values = 1
Masked values: Values(ACTIVE) | ~Defined(ACTIVE) : 111111
Masked values: Values(ERASE) | ~Defined(ERASE)   : 111111
Result of masked &: 111111
Reset values not defined: 111111 & ~Defined(ACTIVE & ERASE)  : 001010

Or at least that is the conclusion I draw from our previous discussions. We could also consider that the single-process operations do not work like cross-process synchronization, but I do not know if that is what we want.

[pooyan-dadvand]

Now I see, thanks for the explanation. It's an interesting side effect!

[pooyan-dadvand]

We should talk about it again

[jcotela]

After trying to implement the proposed behavior, I have found out that it is impossible (given the current interface and the flag operations given above) to apply mask-type operations to flags. For example, if I want only to communicate one flag value (say ACTIVE) from a Flags variable, I would get it as my_flags & ACTIVE, which would return just the ACTIVE bit in a normal flag but returns all set values in my_flags with the proposed logic. Since I believe that this is needed (and that a naive user of our flags would expect to be able to use them in this way), I would drop the behavior I proposed above in favor of a more intuitive one.

The one that @pooyan-dadvand suggested above is probably the simplest behavior that I can think of that still behaves intuitively. Essentially, it implies "when combining (and/or) two flags, all set bits on either flag are set on the result, unset values are taken as false when computing the result of the and/or". The MPI operations should still work as outlined above, however, but this I can "hide" in the internals of the synchronization call.

In practice, this would imply keeping most of the current flags implementation, except for the current &=, which unsets flags that are not set on the RHS (I understand that this is a bug, according to @pooyan-dadvand).

Side note (mainly for @pooyan-dadvand): I have been looking into three-valued logic systems (true-false-undefined) but the ones you can find in books usually define & to imply undefined & true = undefined, which is not what we want for MPI communication. I would not adopt them, since we would need to define a custom define-& operation in addition to the natural one.

[pooyan-dadvand]

Thanks @jcotela for your precise description of the situation. Now I see why three-valued logic makes undefined & true = undefined which is more coherent with the mask behavior. Actually I'm not sure if my implemented behavior (base on C flags) is better than the three-valued ones...

It would be great to have some opinions from @KratosMultiphysics/implementation-committee and @KratosMultiphysics/design-committee about the three options of dealing with &:

1. Making "and" of flag bits and "or" of defined bit: true & (undefined) -> false (defined)
2. Not defined no vote: true & (undefined) -> true (defined)
3. Three-valued logic: true & (undefined) -> (undefined)

[philbucher]

We discussed this in the @KratosMultiphysics/design-committee and reached a consensus: we prefer option 1: "Making "and" of flag bits and "or" of defined bit: true & (undefined) -> false (defined)"

[pooyan-dadvand]

@KratosMultiphysics/technical-committee agrees with the decision of @KratosMultiphysics/design-committee