[ModelPart] CreateNewElement/-Condition sorts Nodes

[philbucher]

Description
CreateNewElement and CreateNewCondition sort the Nodes when the nodes are given by Ids, i.e. when using this function:

Kratos/kratos/includes/model_part.h

Lines 1069 to 1072 in 5009962

	/// Creates new element with a node ids list.
	ElementType::Pointer CreateNewElement(std::string ElementName,
	IndexType Id, std::vector<IndexType> ElementNodeIds,
	PropertiesType::Pointer pProperties, IndexType ThisIndex = 0);

If the Nodes are directly given then it works fine, i.e. when using this function:

Kratos/kratos/includes/model_part.h

Lines 1074 to 1077 in 5009962

	/// Creates new element with a nodes list.
	ElementType::Pointer CreateNewElement(std::string ElementName,
	IndexType Id, Geometry< Node < 3 > >::PointsArrayType pElementNodes,
	PropertiesType::Pointer pProperties, IndexType ThisIndex = 0);

The problems seems to be that the nodes are retrieved by Id using pGetNode:

Kratos/kratos/sources/model_part.cpp

Line 948 in 5009962

pElementNodes.push_back(pGetNode(ElementNodeIds[i]));

pGetNode calls find in the Mesh:

Kratos/kratos/includes/mesh.h

Line 309 in 5009962

auto i = mpNodes->find(NodeId);

This call ends up in the PointerVectorSet where the offending Sort is called (only in the non-const version of find):

Kratos/kratos/containers/pointer_vector_set.h

Line 484 in 5009962

Sort();

IMO this sort is wrong. I would not expect my set to be sorted when I call find.

Rationale: IMO the sort should be removed from the find function, i.e. the non-const function should do the same as the const one

Scope

• KratosCore => ModelPart

To Reproduce

Model model;
auto& model_part = model.CreateModelPart("mp");

model_part.CreateNewNode(2,0,0,0);
model_part.CreateNewNode(1,0,0,0);

std::cout << "Node Ids before creating element:" << std::endl;
for (const auto& r_node : model_part.Nodes()) {
    std::cout << "Node Id: " << r_node.Id() << std::endl;
}

auto p_props = model_part.CreateNewProperties(0);

std::vector<IndexType> connectivity {2,1};

model_part.CreateNewElement( // or CreateNewCondition
    "Element2D2N",
    0,
    connectivity,
    p_props
);

std::cout << "Node Ids after creating element:" << std::endl;
for (const auto& r_node : model_part.Nodes()) {
    std::cout << "Node Id: " << r_node.Id() << std::endl;
}

This prints:

Node Ids before creating element:
Node Id: 2
Node Id: 1
Node Ids after creating element:
Node Id: 1
Node Id: 2

Expected behavior
CreateNewElement and CreateNewCondition should not sort the Nodes

@KratosMultiphysics/technical-committee

[RiccardoRossi]

rom my point of view the find does not work if the nodes are not sorted...so the sorting is an expected side effect i would say...

the problem is that the Nodes array should always be sorted, if it is not it implies we are doing something wrong before

[philbucher]

rom my point of view the find does not work if the nodes are not sorted...so the sorting is an expected side effect i would say...

but it works if it is const

PointerVectorSet my_set;
// ...
my_set.find(...); // this might do the Sort

const PointerVectorSet& const_ref = my_set;
const_ref.find(...); // this does not do Sort

the problem is that the Nodes array should always be sorted, if it is not it implies we are doing something wrong before

why?

[RiccardoRossi]

the problem is probably the buffer size in PointerVectorSet.

the PointerVectorSet probably needs a good rewrite/cleanup

[loumalouomega]

the problem is probably the buffer size in PointerVectorSet.

the PointerVectorSet probably needs a good rewrite/cleanup

+100

[loumalouomega]

rom my point of view the find does not work if the nodes are not sorted...so the sorting is an expected side effect i would say...

You can create a temp vector

the problem is that the Nodes array should always be sorted, if it is not it implies we are doing something wrong before

The order of the ids should be the one of the connectivity? (which is not necessarily ordered)

[pooyan-dadvand]

I believe that we should take out the buffer (Which I can do by myself)

[roigcarlo]

@KratosMultiphysics/technical-committee have decided to add a new threadsafe method CreateNewNodeWithoutAdding(id, x, y, z) which will allow to create new nodes without adding them to the modelpart while still using its mpVariableList and mBufferSize`, and will be able to be added once all are created.

[loumalouomega]

@KratosMultiphysics/technical-committee have decided to add a new threadsafe method CreateNewNodeWithoutAdding(id, x, y, z) which will allow to create new nodes without adding them to the modelpart while still using its mpVariableList and mBufferSize`, and will be able to be added once all are created.

Question?, but you will be the responsible to store those nodes or will be stored somewhere?

[roigcarlo]

You will be responsible to store those until added to the modelpart. Mainly that would let you create a bunch of nodes without having to call the internal sort of the PointerVectorSet.

[loumalouomega]

You will be responsible to store those until added to the modelpart. Mainly that would let you create a bunch of nodes without having to call the internal sort of the PointerVectorSet.

So this is mostly a macro, because the ModelPart does not do anything special in order to create nodes

[loumalouomega]

You will be responsible to store those until added to the modelpart. Mainly that would let you create a bunch of nodes without having to call the internal sort of the PointerVectorSet.

So this is mostly a macro, because the ModelPart does not do anything special in order to create nodes

The only thing special is the VariableLists

[philbucher]

@KratosMultiphysics/technical-committee have decided to add a new threadsafe method CreateNewNodeWithoutAdding(id, x, y, z) which will allow to create new nodes without adding them to the modelpart while still using its mpVariableList and mBufferSize`, and will be able to be added once all are created.

but how does this solve the issue? I don't think adding tricky-to-use interfaces to the (already blob object) ModelPart will help here
IMO this is an issue of the PointerVectorSet

[loumalouomega]

@KratosMultiphysics/technical-committee have decided to add a new threadsafe method CreateNewNodeWithoutAdding(id, x, y, z) which will allow to create new nodes without adding them to the modelpart while still using its mpVariableList and mBufferSize`, and will be able to be added once all are created.

but how does this solve the issue? I don't think adding tricky-to-use interfaces to the (already blob object) ModelPart will help here IMO this is an issue of the PointerVectorSet

[pooyan-dadvand]

@KratosMultiphysics/technical-committee agrees that the issue is from PointerVectorSet and to be fixed by taking out the buffer.

The method of ModelPart is a side discussion for solving performance issue that we have in creation of nodes and we do it manually in several places and taking out the buffer will even worsen it.

[philbucher]

another problem I discovered: when accessing the (non-const) PointerVectorSet in parallel with find, then it might Sort which causes the parallel loop to crash. No problem if doing the same with a const PointerVectorSet