Kratos: [Core] Delete Debug Errors in Elements for nullptr in GetProperties and pGetProperties

[tteschemacher]

Can we delete the Debug Errors in the elements? Lines 1027-1057. The same would then account for the conditions.

PropertiesType::Pointer pGetProperties()
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return mpProperties;
}

const PropertiesType::Pointer pGetProperties() const
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return mpProperties;
}

PropertiesType& GetProperties()
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return *mpProperties;
}

PropertiesType const& GetProperties() const
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return *mpProperties;
}

It should be possible to also access to a nullptr if the property is not applied to the element. This can be necessary if one element is copied with all source entities. As an element can exist without property this should be possible to be copied.

@KratosMultiphysics/implementation-committee

[loumalouomega]

Why do you want to remove that?

[tteschemacher]

I first create elements to later create the needed elements from Reference Elements. It is a Elements to Elements or Conditions function. There, I can live quite well with copying the nullptr. But still the function should be generic that I could also allow changes from later elements to other elements.

const Element& rReferenceElement = KratosComponents<Element>::Get(rElementName);

auto p_element = rReferenceElement.Create(
     rIdCounter, it_elem->pGetGeometry(), it_elem->pGetProperties());

[loumalouomega]

OK, this will be discussed by the @KratosMultiphysics/implementation-committee

[philbucher]

could be replaced by a debug-warning ...?

@jcotela added them in #1348, maybe he can comment on it

[pablobecker]

@tteschemacher could you clarify why you need to do this? do you have an example where this is used and the code is crashing in debug?
from the @KratosMultiphysics/implementation-committee , we advise against removing this error, furthermore,it should also be in release mode to ensure consistency and detecting errors there.,

[tteschemacher]

I want to create elements with the following command, without differencing whether they have a cl or whether not. This is NOT an error because elements can exist without CL.

auto p_element = rReferenceElement.Create(
rIdCounter, it_elem->pGetGeometry(), it_elem->pGetProperties());

[RiccardoRossi]

...can't you pass an empty property? note that propery and CL is not the same

[philbucher]

...can't you pass an empty property? note that propery and CL is not the same

I think this should work

furthermore,it should also be in release mode to ensure consistency and detecting errors there.,

@pablobecker @KratosMultiphysics/implementation-committee I disagree abt adding also in release since this is used very often and is not useful enough (i.e. in most cases you have the properties properly initialized so no need to constantly check it) to justify the performance hit

[tteschemacher]

...can't you pass an empty property? note that propery and CL is not the same

One could do that, also possible to pass a nullptr. But to keep the function generic and either passing the property or if not needed, not passing it.

In the analysis staged IO, first, the geometry is read in. Later, the properties are enhanced. If one wants to do operations in between, this error would make this impossible.

[AlejandroCornejo]

The members of the @KratosMultiphysics/implementation-committee have reconsidered their position with respect to this issue. We now recommend turning all the errors into Debug-mode warnings according to the comments by @tteschemacher and @philbucher above. Waiting for the input from @adityaghantasala to confirm consensus.

[RiccardoRossi]

@KratosMultiphysics/technical-committee has been discussing this, and we are convinced that throwing this error is correct.

Nevertheless, can u point to a concrete example in which this error becomes a blocker? (we mean without the simple workaround of creating an empty property)

[adityaghantasala]

@KratosMultiphysics/implementation-committee ... I think adding a debug warning in the two functions

PropertiesType::Pointer pGetProperties()
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return mpProperties;
}

const PropertiesType::Pointer pGetProperties() const
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return mpProperties;
}

is fine. But in the other two

PropertiesType& GetProperties()
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return *mpProperties;
}

PropertiesType const& GetProperties() const
{
    KRATOS_DEBUG_ERROR_IF(mpProperties == nullptr)
        << "Tryining to get the properties of " << Info()
        << ", which are uninitialized." << std::endl;
    return *mpProperties;
}

should be an error otherwise it will segfault at the return statement. Which should be avoided.

[jcotela]

I agree with @adityaghantasala's position above. We can be a bit more relaxed with the pointer methods and leave it to the caller to check that the pointer is not null, but the reference versions should be errors.

[AlejandroCornejo]

The @KratosMultiphysics/implementation-committee is waiting for the suggested implementation regarding the pointers/references

[RiccardoRossi]

ping

[RiccardoRossi]

just to resume: @KratosMultiphysics/technical-committee also agrees on the proposed solution (error if getting references, no error otherwise).

[philbucher]

just to resume: @KratosMultiphysics/technical-committee also agrees on the proposed solution (error if getting references, no error otherwise).

@KratosMultiphysics/technical-committee also said that we will see if removing the error gives problems, in which case we would go back to the error, then you would need to work with dummy properties

[AlejandroCornejo]

ping @tteschemacher. In what state of development is this issue? The @KratosMultiphysics/implementation-committee had assigned its development to @tteschemacher since this developer was the one that proposed it. Are you still able to go on it?

[tteschemacher]

I can make an PR adressing this, asap.

[tteschemacher]

I added #5634 to adress this issue. As soon as this is approved we can close this issue.

Thanks all for the comments!