Unify behaviour of GetSolutionStepValue and FastGetSolutionStepValue

[jrubiogonzalez]

Hi,

Right now we have 2 methods that perform mostly the same, the FastGet skips a check that has quite an impact in performance. On the other hand while dubgging this check is quite useful, and unfortunately once your application is runnig smooth it cannot be changed using a replace all.

My proposal is to unify behaviour of both methods and decide to skip the check or not using a flag. This would help us while debugging and also provide an easy way of incresing performance in the final application.

What do you think?
@roigcarlo , @RiccardoRossi , @jcotela , @pooyan-dadvand, @pablobecker @ddiezrod @rubenzorrilla @loumalouomega

[ddiezrod]

There might be historical reasons for this but I also think they should be unified. Their use along the code is quite random anyway. Other option could be to execute the check only in debug mode.

[loumalouomega]

Agree with @ddiezrod

[RiccardoRossi]

with current implementation FastGetSolutionStepValue does the checks when KRATOS_DEBUG is defined, however it does not in release.

the nice thing is that if you use the GetSolutionStepValue you also have the checks in release mode, which is a nice-to-have feature

[pablobecker]

Ricc, then when is it a good idea to use it?

…

On Mon, 19 Nov 2018 at 19:48, Riccardo Rossi ***@***.***> wrote: with current implementation FastGetSolutionStepValue does the checks when KRATOS_DEBUG is defined, however it does not in release. the nice thing is that if you use the GetSolutionStepValue you also have the checks in release mode, which is a nice-to-have feature — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3411 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYf5iZAVTNhwY2oafCiSFEO71cXfo-eDks5uwvzmgaJpZM4YpJTA> .

[RiccardoRossi]

for example from python, where you anyway have an overhead, the function GetSolutionStepValue is used in the interface.

if you have a check or something that is done very seldom, then that is a correct place to use the "Get" version, elsewhere in performance-sensitive code it is good to use the "FastGet"

[philbucher]

Thanks for the explanation @RiccardoRossi
It seems to me like it already works as intended

[pooyan-dadvand]

@RiccardoRossi gives the correct explanation. Nevertheless we can add the test in the python interface and unify both of them. I agree with @jrubiogonzalez that its usage in cpp is not well described nor uniform.

I would go for unifying them and in few cases that I used them to check the coherence (basically in IO) I can add the check manually.

[jrubiogonzalez]

In some way we should probably discourage the usage of non Fast* in C++ side.

[pooyan-dadvand]

Forwarding to @KratosMultiphysics/implementation-committee

[loumalouomega]

Which is the conclussion then?

[jcotela]

@loumalouomega essentially, that we would like to hear the opinion of the @KratosMultiphysics/implementation-committee

[loumalouomega]

I think that the current implementation is fine, as @RiccardoRossi said the check is included with the debug mode, which is fine

[AlejandroCornejo]

Has been interesting to know how this feature exactly works , thank you @RiccardoRossi ! If the implementation of the FastGet and Get does as expected, my vote in the @KratosMultiphysics/implementation-committee is to remain as it is, as @loumalouomega says...

[AlejandroCornejo]

maybe could be useful to inform everybody about the behaviour of this methods in the wiki??

[pooyan-dadvand]

I agree to put this explanation in wiki but I don't think it would significantly improve the situation. Note that even our experienced developers are not aware of the difference. So I would consider the suggestion of @jrubiogonzalez:

In some way we should probably discourage the usage of non Fast* in C++ side.

[philbucher]

we could deprecate the non-fast version

[loumalouomega]

we could deprecate the non-fast version

We can discuss that, @KratosMultiphysics/implementation-committee

[GuillermoCasas]

Another option, in line with the request of @jrubiogonzalez, would be to change the C++-side function name to 'SlowGetSolutionStepValue' or 'SecureGetSolutionStepValue'; forcing people to check the wiki in case they want to use it.

[loumalouomega]

Another option, in line with the request of @jrubiogonzalez, would be to change the C++-side function name to 'SlowGetSolutionStepValue' or 'SecureGetSolutionStepValue'; forcing people to check the wiki in case they want to use it.

I don't know if it is a good idea to add new names for the same methods, it will increase the confussion

[GuillermoCasas]

@loumalouomega Yes, true, but... Perhaps renaming GetSolutionStepValue to SecureGetSolutionStepValue, and then FastGetSolutionStepValue to GetSolutionStepValue in the C++ and deprecating FastGetSolutionStepValue would work.

The behaviour would be the same as now: the python version (only GetSolutionStepValue would be provided) would be the secure one by default, calling SecureGetSolutionStepValue under the hood. Meanwhile, on the C++ side the default, fast version, would surely be the one used by most developers.

There would not be much confusion: call GetSolutionStepValue from python and C++ always, except when you specifically need to call SecureGetSolutionStepValue on the C++ side.

[AlejandroCornejo]

In my opinion I suggest to only use Fast in c++ and Get in python only... as says @philbucher

[loumalouomega]

In my opinion I suggest to only use Fast in c++ and Get in python only... as says @philbucher

+1

[pooyan-dadvand]

In my opinion I suggest to only use Fast in c++ and Get in python only... as says @philbucher

I agree but then I would rename it to the GetValue and force the check in Python interface implementation. In this way we are clearly saying that we do the check only for Python.

[GuillermoCasas]

To the @KratosMultiphysics/technical-committee: the @KratosMultiphysics/implementation-committee has arrived at the consensus that replacing 'FastGetSolutionStepValue' by GetSolutionStepValue and doing the check in debug mode only is the right goal. This should be done in two steps:

1. Make GetSolutionStepValue have this behaviour, replace all visible 'FastGetSolutionStepValue' calls and add a warning to inform the remaining users
2. Remove 'FastGetSolutionStepValue' definitively.

[loumalouomega]

Yes.

[philbucher]

Yes.

nice :D

[loumalouomega]

Removing the label until the 5/5 is reach

[GuillermoCasas]

Ok, so we reached the following recommendation about this issue:

GetSolutionStepValue should replace FastGetSolutionStepValue everywhere, while keeping the fast implementation for all cases except:

1. Python (it is already already slow, so we do not gain much from removing the check here)
2. Everywhere in debug mode

We are aware of the difficulty of doing this massive migration without causing too much trouble. We have suggested (see comments above) to break the migration up into two stages. The first stage would imply replacing as many calls as possible and add a warning to the deprecated calls (or perhaps just a compilation warning), while maintaining the 'FastGetSolutionStepValue' support.

Passing this on to the @KratosMultiphysics/technical-committee.

[philbucher]

I am still a bit confused how you plan to do the python-implementation without doing the check twice when compiled in Debug:

If you expose GetSolutionStepValue (which as far as I understand will do the check only in Debug) then you will not have the Check in Release
If you add an interface-method, that does the check also in release, then in Debug the check will be done twice (in the Container itself and in the interface-function)

Could you please explain a bit more in detail?

[loumalouomega]

Basically the check in python is done only in Release, so when in Debug the check will be done in the inner c++ method

[GuillermoCasas]

@philbucher You are right. We figured that if you are on debug mode it would be OK to have the check done twice. But it is true that one could also skip the check in the python layer when in debug mode since one knows it will be performed in the inner function. We did not go into that much detail but perhaps it is worth an additional round of discussion of the @KratosMultiphysics/implementation-committee. I am removing the consensus tag for now.

[philbucher]

Basically the check in python is done only in Release, so when in Debug the check will be done in the inner c++ method

this would work I think @GuillermoCasas

so in add_xxx_to_python you could check if compiled in debug or not and add the check depending on that

[GuillermoCasas]

@philbucher Yes, let us see what the rest think about this.

[RiccardoRossi]

this is just to say that i am not in favor of removing the Fast method

[maceligueta]

Hi Riccardo. Can you elaborate? It is relatively easy to replace the method.

[pablobecker]

@maceligueta . Out of curiosity, since you work with explicit methods more than we do, how does the speed of the FastGet compare to the Get... (with the checks) .?

[maceligueta]

We never did that comparison...

[pablobecker]

If I recall correctly, @ddiezrod did the comparison with an implicit solver and there was no difference. But time is dominated by system solve here, so we can't extract conclusions from this.

[RiccardoRossi]

My position is that i would prefer Keeping the two methods as now:
1 - GetSolutionStepValue (always doing the checking)
2 - FastGetSolutionStepValue (doing the check only in debug mode).

This way one may deliberately ensure that some things are checked even in release

[pooyan-dadvand]

@KratosMultiphysics/technical-committee agrees with @KratosMultiphysics/implementation-committee about:

1. Python (it is already already slow, so we do not gain much from removing the check here)
2. Everywhere in debug mode

The idea is to have this change in 3 phases:

1. Changing the behavior of the GetSolutionStepValue to only debug check. Adding the check in Python interface and in some places like IO that we want really check it in release.
2. In a separate PR changing all FastGetSolutionStepValue to GetSolutionStepValue and add a compiler error in FastGetSolutionStepValue definition.
3. After a period of time we can remove the FastGetSolutionStepValue method.