[Core] Replace geometry container with PVS #13377

rickyaristio · 2025-04-28T15:24:01Z

📝 Description
This PR serves two main goals:

Replace GeometryContainer with PointerVectorSet:
As discussed in GeometryContainer #11609 in order to have a standard interface.
Fully remove GeometryContainer:
Following up on [Core] Use PointerVectorSet as geometry container #12843, this PR fully removes the GeometryContainer and aliases it to the PointerVectorSet of the geometries.

@sunethwarna @matekelemen

matekelemen

Awesome, thanks Ricky! This'll definitely need a peek from the TC though.

juancamarotti

Nice work Ricky! All the CI Tests passed, so it's completely fine from my side. Thanks a lot

rickyaristio · 2025-04-29T07:08:10Z

Nice work Ricky! All the CI Tests passed, so it's completely fine from my side. Thanks a lot

I will wait for the approval from the TC, as this involves changes in the core.

RiccardoRossi

guys, who approved this? it should be approved by the @KratosMultiphysics/technical-committee

matekelemen · 2025-04-30T14:18:25Z

guys, who approved this? it should be approved by the @KratosMultiphysics/technical-committee

That's what we're waiting for.

An approval is just that: someone expressing support for the PR. It doesn't mean the PR is merged.

The fact that the PR is mergable without approval from the technical committee is the fault of an improperly set up code owner file.

sunethwarna · 2025-04-30T14:51:16Z

I would say this is a good opportunity to move the GeometryContainer to the Mesh and be consistent with all the other containers in ModelPart, unless there is a specific reason for it to be in ModelPart. @KratosMultiphysics/technical-committee ?

kratos/sources/model_part.cpp

pooyan-dadvand · 2025-05-14T13:04:30Z

kratos/sources/model_part.cpp

+            // Check that the connectivities are the same
+            // note that we deliberately check the node ids and not the pointer adresses as there might be very rare situations
+
+            // (e.g., creating nodes bypassing the model part interface) with same connectivities but different pointer addresses


I didn't understand why pointer would fail. Nevertheless, checking the Id is ok for me.

I think they're mimicking what we did for elements/conditions not many years ago (I remember discussing this in the @KratosMultiphysics/technical-committee). Am I wrong @rickyaristio?

Edit: well, I think I remember...

@rubenzorrilla , you are right.

loumalouomega · 2025-05-14T13:07:25Z

I am modifying these files, this is going to be a conflict :P

kratos/sources/model_part.cpp

rubenzorrilla · 2025-05-22T08:24:06Z

kratos/sources/model_part_io.cpp

@@ -1812,7 +1810,7 @@ void ModelPartIO::ReadGeometriesBlock(NodesContainerType& rThisNodes, GeometryCo
            temp_geometry_nodes.push_back( *(FindKey(rThisNodes, ReorderedNodeId(node_id), "Node").base()));
        }

-        rThisGeometries.AddGeometry(r_clone_geometry.Create(ReorderedGeometryId(id), temp_geometry_nodes));
+        rThisGeometries.insert(r_clone_geometry.Create(ReorderedGeometryId(id), temp_geometry_nodes)); //TO DO!


What is exactly the remaining TODO?

One remaining problem with geometries is their IDs.

It's not the straight and simple index-based ID we have on everything else, but a hash that encodes some unrelated data in the two most significant bits.

I imagine he tagged every place that uses geometry IDs.

well...this is NOT insignificant eh ... actually it is the reason for which we were using a special container

One remaining problem with geometries is their IDs.

It's not the straight and simple index-based ID we have on everything else, but a hash that encodes some unrelated data in the two most significant bits.

I imagine he tagged every place that uses geometry IDs.

guys this is a killer...

guys this is a killer...

Why? The special container (a hash table) has been swapped out for PVS for over a year, and we never saw a performance regression.

The only reason for the hash-based IDs is the option to construct a Geometry without providing an ID (in that case, it gets automatically generated). The last two bits store information on how the ID was assigned: provided or generated.

I don't see why this ID auto-generation feature is necessary. On the other hand, it creates quite a few headaches:

the hash function does not guarantee uniqueness, so we're always running the risk of silent bugs

Geometry IDs are not consistent between runs (the hashing is unique to each run)

they are discontiguous, huge integers, terrible for debugging

bit-twiddling magic that is hard to read/understand/maintain

@RiccardoRossi why do you think auto-generated IDs are necessary? I don't see a valid use case.

well, the use case was to identify as given triangle (say) but its connectivity. If this is not any longer supported than my review does not make sense.

If anything used that feature, it would have broken after merging #12843.

rubenzorrilla · 2025-05-22T08:24:43Z

kratos/tests/cpp_tests/containers/test_geometry_container.cpp

@@ -1,177 +0,0 @@
-//    |  /           |


Can't we reuse all these as tests for the PVS?

Let me try it after we move the geometry container to mesh (so it is consistent with the elements/conditions). #13423

rubenzorrilla

I only have a few minor observations. Aside of these, I don't see any error.

As a comment aside, I think that at some point we should consider doing a search&replace of all the GeometryName as these are very misleading. IMO, GeometryName in Kratos context means Triangle2D3, Line2D3, etc. What we have in here should be GeometryTextId or something of this sort.

roigcarlo

👍 Looks good from my side as well

not any longer valid

Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>

rickyaristio added 4 commits April 28, 2025 17:04

change geometry container type to PVS

4b05f1b

modify the member functions due to changes in the geometry container

61797e0

modify the geoemtry container type

4b800df

remove geoemtry container and its test

88a1ff9

rickyaristio requested a review from a team April 28, 2025 15:24

rickyaristio assigned sunethwarna and matekelemen Apr 28, 2025

rickyaristio requested a review from a team as a code owner April 28, 2025 15:24

matekelemen reviewed Apr 28, 2025

View reviewed changes

juancamarotti previously approved these changes Apr 29, 2025

View reviewed changes

RiccardoRossi previously requested changes Apr 30, 2025

View reviewed changes

RiccardoRossi added this to Technical Commiittee Apr 30, 2025

matekelemen requested a review from a team April 30, 2025 14:29

sunethwarna moved this to 👀 Next meeting TODO in Technical Commiittee Apr 30, 2025

sunethwarna moved this from 👀 Next meeting TODO to 👓 Check for the next meeting in Technical Commiittee May 8, 2025

rickyaristio mentioned this pull request May 13, 2025

[Core] Move geometry container to mesh #13423

Open

pooyan-dadvand reviewed May 14, 2025

View reviewed changes

rubenzorrilla reviewed May 22, 2025

View reviewed changes

kratos/sources/model_part.cpp Outdated Show resolved Hide resolved

rubenzorrilla reviewed May 22, 2025

View reviewed changes

roigcarlo reviewed May 22, 2025

View reviewed changes

Update kratos/sources/model_part.cpp

68abb32

Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>

rickyaristio dismissed juancamarotti’s stale review via 68abb32 May 30, 2025 14:09

[Core] Replace geometry container with PVS #13377

Are you sure you want to change the base?

[Core] Replace geometry container with PVS #13377

Conversation

rickyaristio commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matekelemen left a comment

Choose a reason for hiding this comment

Uh oh!

juancamarotti left a comment

Choose a reason for hiding this comment

Uh oh!

rickyaristio commented Apr 29, 2025

Uh oh!

RiccardoRossi left a comment

Choose a reason for hiding this comment

Uh oh!

matekelemen commented Apr 30, 2025

Uh oh!

sunethwarna commented Apr 30, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenzorrilla May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loumalouomega commented May 14, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matekelemen May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matekelemen May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickyaristio May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenzorrilla left a comment

Choose a reason for hiding this comment

Uh oh!

roigcarlo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rickyaristio commented Apr 28, 2025 •

edited

Loading

rubenzorrilla May 22, 2025 •

edited

Loading

matekelemen May 22, 2025 •

edited

Loading

matekelemen May 24, 2025 •

edited

Loading

rickyaristio May 30, 2025 •

edited

Loading