Kratos: Non const Get functions in base classes (element.h, condition.h, etc.)

[inigolcanalejo]

@RiccardoRossi , @pooyan-dadvand or anyone that can answer this question,

I observed there are several Get functions defined in element.h (e.g. GetValuesVector, GetValueOnIntegrationPoints), which are not const. I thought the nature of a Get function is to get some values without changing them and therefore it should be defined as const to avoid problems. What is the reason for not doing it like this? Flexibility?

This came up because I have a function (CalculateGradient) that inputs a constant element (because again, we do not want to change it) and of course, I cannot call GetValuesVector because it is not defined as const.

Solutions we came up with together with @msandre :

1. Just not define the function CalculateGradient to input a constant element, but just an element (and risk that the element can be changed inside this function)

2. const_cast the element when calling the non const GetValuesVector

3. Define another GetValuesVector in element.h which is const. This is actually is done for: GetProperties and GetValue, which are defined twice: once as const and once as non const (why?)

4. Change all Get functions to const (which will involve overriding them again as const in all of the applications)

Any ideas how to proceed or explanation why it is done like this?

Thank you in advance.

[jcotela]

Hi @inigolcanalejo I seem to recall that these methods could not be const due to missing const versions of operations commonly used within them (I recall some missing const access to the nodes, but I might be wrong). Can you try to define a const version yourself of (for example) GetValuesVector or GetValueOnIntegrationPoints for your element and see if you get compilation errors?

Regarding your point 3, methods that have const and non-const versions typically exist because the underlying access also has const and non-const versions. For example GetProperties should return different things in each version: one a const pointer/reference, the other a regular one, while the Get operation in GetValue has different meaning in each case: the non-const version initializes the container to zero if the requested variable was not initialized before (and therefore is not threadsafe!!! Do not use it within openmp loops), the const one doesen't.

[inigolcanalejo]

Hi @jcotela thank you for your detailed answer. I tried defining a const version myself as you suggested (i.e. following point 3), it compiles without errors and works smoothly in a small test. Thus I will use this.

[jcotela]

Hi @inigolcanalejo, to be honest, I expected this to fail... if it compiles, I would suggest making a note of which new functions you would be adding/re-defining in element.h so that we can discuss them in the eventual PR.
On one hand, I can see how we might want to eventually remove some of the non-const variants of the methods you mention if there is not an advantage to them (which thanks to the overload keyword in C++11 should be easier to do without introducing errors than in the past).
On the other, we have refrained in the past to do precisely this change (making existing methods const) because changing function signatures has a lot of potential for new and surprising bugs...

@pooyan-dadvand @RiccardoRossi do you think this would be the right approach or should we really avoid touching element.h as much as possible?

[RiccardoRossi]

Hi,

I think that now that we added the override keywords we are a little safer for making this changes, even in the base class. however, can you point to the offending code so to have a better idea of what is going wrong?

[inigolcanalejo]

Hi,

@RiccardoRossi this is the code in question:

In includes/element.h there is this non-const function:

virtual void GetValuesVector(Vector& values, int Step = 0)
{
}

which is overridden in CompressiblePotentialFlowApplication/custom_elements/compressible_potential_flow_element.h to get the nodal data:

void GetValuesVector(Vector& values, int Step = 0) override
{
    //gather nodal data
    for(unsigned int i=0; i<NumNodes; i++)
    {
        values[i] = GetGeometry()[i].FastGetSolutionStepValue(POSITIVE_FACE_PRESSURE);
    }
}

In AdjointFluidApplication/custom_utilities/response_function.h there is this other another function that takes constant Element as an input:

virtual void CalculateGradient(const Element& rAdjointElem,
                               const Matrix& rAdjointMatrix,
                               Vector& rResponseGradient,
                               ProcessInfo& rProcessInfo)
{
}

which is overridden in AdjointFluidApplication/custom_utilities/potentialflow_lift_objective_function.h:

 void CalculateGradient(const Element& rAdjointElem,
    const Matrix& rAdjointMatrix,
    Vector& rResponseGradient,
    ProcessInfo& rProcessInfo) override
{
    KRATOS_TRY;        
    ...
    rAdjointElem.GetValuesVector(Potential,0);
    ...        
    KRATOS_CATCH("");
}

This gives the following error because rAdjointElem is constant and GetValuesVector is not:

error: no matching function for call to ‘Kratos::Element::GetValuesVector(Kratos::Vector&, int) const’
rAdjointElem.GetValuesVector(Potential,0);

note: candidate: virtual void Kratos::Element::GetValuesVector(Kratos::Vector&, int)
virtual void GetValuesVector(Vector& values, int Step = 0)

/home/inigo/software/Kratos/kratos/includes/element.h:322:18: note: passing ‘const Kratos::Element*’ as ‘this’ argument discards qualifiers

The error is solved by adding the constant version of GetValuesVector to elemen.h:

virtual void GetValuesVector(Vector& values, int Step = 0)
{
}

/**
* Getting method to obtain the variable which defines the degrees of freedom
*/
virtual void GetValuesVector(Vector& values, int Step = 0) const
{
}

Of course this also needs to be overridden properly in CompressiblePotentialFlowApplication/custom_elements/compressible_potential_flow_element.h to get the nodal data:

void GetValuesVector(Vector& values, int Step = 0) const override
{
    //gather nodal data
    for(unsigned int i=0; i<NumNodes; i++)
    {
        values[i] = GetGeometry()[i].FastGetSolutionStepValue(POSITIVE_FACE_PRESSURE);
    }
}

@RiccardoRossi do you think there is any problem defining these Get functions twice, once as const and once as non-const?

Thank you all for your help!

[inigolcanalejo]

@jcotela I dont know where to make a note so I will indicate first here which functions I would suggest to:

• first just add as const
• and then eventually remove the non-const

(which are basically all the get functions from element.h and condition.h):

• GetDofList
• GetValuesVector
• GetFirstDerivativesVector
• GetSecondDerivativesVector
• GetValueOnIntegrationPoints

As far as I know, it would be good practice to make those constant.

[jcotela]

I agree with most of these, except with GetValueOnIntegrationPoints. There we sometimes do some on-the-spot calculations that might or might not be stored on an elemental variable. Also note that there is some discussion going on regarding merging GetValueOnIntegrationPoints with CalculateOnIntegrationPoints (so I would be more inclined to just remove it and change the name to the other variant, which has a name that does not imply const-ness).

[inigolcanalejo]

Ok, thanks @jcotela , I will have a look also at the other discussion.

[RiccardoRossi]

Guys, take a look to this link.

https://stackoverflow.com/questions/8645760/avoiding-code-duplication-in-const-and-non-const-member-functions

Maybe we could do it as described ?

[msandre]

We could probably add

• EquationIdVector

  to the list above.

[RiccardoRossi]

Hi Mike,

In any case before doing any move in this direction, it would be important
that all the overrides are added to all of th application s (clang tidy can
do it automwtixally., There is an how-to in the wiki).

Also this is a sort of disruptive change, so let s also wait for Pooyan's
feedback

Riccardo

El 27 ago. 2017 9:01 a. m., "Michael Andre" [e-mail]
escribió:

We could probably add

• EquationIdVector

to the list above.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/732#issuecomment-325183372,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHr7EWfeAWOU3ToMRmqnpw2rlcOcvsd9ks5scSJHgaJpZM4O-ZzU
.

[pooyan-dadvand]

I completely agree with adding the constant version of corresponding get methods. About taking out the non constants, I don't agree with doing it for all of them. Specially the GetValue ones has been used as lhs in many places. (and is not easy to use SetValue everywhere...)

I also agree with @RiccardoRossi that we should add the overrides before removing any method.

[pooyan-dadvand]

To be postponed to the next release...

[philbucher]

done in #4938