[Core] Add new AddNodesFromOrderedContainer to model part
#11503
Conversation
loumalouomega
changed the title
AddNodesFromOrderedContainer to model part
kratos/includes/model_part.h
Outdated
| @@ -399,6 +399,29 @@ class KRATOS_API(KRATOS_CORE) ModelPart final | |||
| KRATOS_CATCH("") | |||
| } | |||
|
|
|||
| template<class TIteratorType > | |||
| void AddNodesFromOrderedContainer(TIteratorType nodes_begin, TIteratorType nodes_end, IndexType ThisIndex = 0) | |||
There was a problem hiding this comment.
ThisIndex is not being used
There was a problem hiding this comment.
Also, very minor, but we use camel case for parameters, and in this case they should start with "i" to indicate its an iterator
There was a problem hiding this comment.
Also input should be in CammelCase format
There was a problem hiding this comment.
I was copying the interface from AddNodes, that TBH has same issue. It is not used. We can Remove both probably.
There was a problem hiding this comment.
there are no guarantees that the output is sorted, apart from the user's good conscience that they provide a sorted input. The fact that PointerVectorSet exposes such manipulation of its underlying data structure is the exact problem detailed in #11363.
This should be handled by a range insert into PointerVectorSet and definitely not implemented from the outside.
| for(TIteratorType it1=bshort; it1!=eshort; it1++) { | ||
| while(it2!=elong && it2->Id()<it1->Id()) { | ||
| aux.push_back(*(it2.base())); | ||
| it2++; | ||
| } | ||
| aux.push_back(*(it1.base()) ); | ||
| if(it2!=elong && (it1->Id() == it2->Id())) { //If both are the same, then we need to skip | ||
| it2++; | ||
| } | ||
| } | ||
| while(it2 != elong) { | ||
| aux.push_back(*(it2.base())); | ||
| it2++; | ||
| } |
There was a problem hiding this comment.
this will still add duplicate nodes if it1 or it2 iterate through duplicate nodes
There was a problem hiding this comment.
This is the exact goal of this MR.The condition is being able to provide unique and sorted nodes. It is quite common to have it as the mesh generation directly/indirectly is under our control.
There was a problem hiding this comment.
To provide some context, we are experimenting with a voxel mesher. And for a benchmark example, with these changes it goes from 70s to 20s. Quite a lot if we want to be in the order 1min.
There was a problem hiding this comment.
Changes in the ModelPart API need to be discussed in the @KratosMultiphysics/technical-committee.
EDIT: Though this is a justified demand, I tend to agree @matekelemen and @sunethwarna point about tackling this from the container itself, on top of the fact that there are many routines in Kratos assuming sorted and unique containers.
If we are to do this from the ModelPart as a sort of temporary patch, I think we should consider a different alternative that eases an eventual migration rather than creating a new function to an already exorbitant API.
|
@KratosMultiphysics/technical-committee understands that the modifications proposed by #12087 can solve the problem of sort and unique and can be used as the low level implementation for this one. However, for adding the entities to the different levels of the modelpart hierarchy, we need a separate utility (not in the modelpart itself) to perform that. |
📝 Description
This PR Adds a new method to the model part. This method greatly improves performance when the container to be added can be ensured to have the nodes only once and are stored ordered.
🆕 Changelog