[Core] Use PointerVectorSet as geometry container #12843

rubenzorrilla · 2024-11-11T15:20:25Z

📝 Description
So far, the GeometryContainer used the PointerHasMapSet as internal container for the geometries. @pooyan-dadvand as we discussed the other day, this PR upgrades it to use the customary PointerVectorSet.

Making it draft until we are sure that all the suites pass (I ran the main ones that might be directly affected by this change).

rubenzorrilla · 2024-11-11T15:28:03Z

@sunethwarna @mpentek as far as I understand, this shouldn't interfere with your current PVS upgrade.

sunethwarna · 2024-11-11T15:33:13Z

FYI: @rickyaristio @tteschemacher

sunethwarna · 2024-11-11T15:33:40Z

@sunethwarna @mpentek as far as I understand, this shouldn't interfere with your current PVS upgrade.

Nice, finally.... :) @matekelemen

tteschemacher · 2024-11-11T16:02:09Z

I'm not sure if that works. The hash map would be indexed by the identifier, while the pointer vector by the index. This is a matter of efficiency of the model becomes large.

rubenzorrilla · 2024-11-12T07:06:07Z

I'm not sure if that works. The hash map would be indexed by the identifier, while the pointer vector by the index. This is a matter of efficiency of the model becomes large.

mmm... I'm not sure I'm getting your point at all. The PointerVectorSet has an undeliying std::vector so the indexing is immediate (provided it is sorted of course).

EDIT: also note that this is what we actually do for elements/conditions, to which your orbservation would also be important.

sunethwarna · 2024-11-12T07:21:48Z

@tteschemacher, as far as I see, the geometry identifier is also an int even though it can be hashed as well, so I don't see how the use of a map could be faster because both the PointerVectorSet and PointetHashMapSet will have the same O(log(N)) complexity if you query using the Id. The complexity will be O(1) if you query using the index for PointerVectorSet and you cannot do the same for the PointerHashMapSet. Am I wrong?

Another note: PointerHashMapSet uses an std::unordered_map in the back end to store data which does not allow random access iterators, this prohibits using any parallelization loops to work on the geometry container which would be a big drawback compared to PointerVectorSet.

sunethwarna

If we are changing this to align with the other containers, then we need to fully remove the GeometryContainer and alias it to the PointerVectorSet of the geometries.

rubenzorrilla · 2024-11-12T10:17:23Z

If we are changing this to align with the other containers, then we need to fully remove the GeometryContainer and alias it to the PointerVectorSet of the geometries.

In principle, makes sense for me. But I'd do it in a second stage (just in case...).

pooyan-dadvand

Just a minor comment

pooyan-dadvand · 2024-11-15T07:56:20Z

kratos/sources/model_part_io.cpp

@@ -288,7 +288,7 @@ void ModelPartIO::WriteGeometries(GeometryContainerType const& rThisGeometries)
        (*mpStream) << "Begin Geometries\t" << geometry_name << std::endl;
        const auto it_geom_begin = rThisGeometries.Geometries().begin();
        (*mpStream) << "\t" << it_geom_begin->Id() << "\t";
-        auto& r_geometry = *(it_geom_begin.base()->second);
+        auto& r_geometry = *(*(it_geom_begin.base()));


shouldn't be: auto& r_geometry = *it_geom_begin

I can give it a try, but I think that this will give you the content of the begin() iterator of the underlying container, which is an std::vector or, in other words, a pointer to the geometry but not a reference to the geometry.

Please give it a try! The PointerVectorSet has its * operator redefined.

You were right! I didn't remember the redefinition. Thanks for the hint.

pooyan-dadvand · 2024-11-15T07:56:24Z

kratos/sources/model_part_io.cpp

@@ -302,7 +302,7 @@ void ModelPartIO::WriteGeometries(GeometryContainerType const& rThisGeometries)
        for(std::size_t i = 1; i < rThisGeometries.NumberOfGeometries(); i++) {
            if(GeometryType::IsSame(*it_geom_previous, *it_geom_current)) {
                (*mpStream) << "\t" << it_geom_current->Id() << "\t";
-                r_geometry = *(it_geom_current.base()->second);
+                r_geometry = *(*(it_geom_current.base()));


pooyan-dadvand · 2024-11-15T07:56:31Z

kratos/sources/model_part_io.cpp

@@ -313,7 +313,7 @@ void ModelPartIO::WriteGeometries(GeometryContainerType const& rThisGeometries)

                (*mpStream) << "Begin Geometries\t" << geometry_name << std::endl;
                (*mpStream) << "\t" << it_geom_current->Id() << "\t";
-                r_geometry = *(it_geom_current.base()->second);
+                r_geometry = *(*(it_geom_current.base()));


pooyan-dadvand · 2024-11-15T07:56:41Z

kratos/utilities/auxiliar_model_part_utilities.cpp

@@ -473,7 +473,7 @@ void AuxiliarModelPartUtilities::RemoveOrphanNodesFromSubModelParts()
                }
                const auto& r_geometries = r_sub_model_part.Geometries();
                for (auto it_geom = r_geometries.begin(); it_geom != r_geometries.end(); ++it_geom) {
-                    auto& r_geometry = *((it_geom.base())->second);
+                    auto& r_geometry = *(*(it_geom.base()));


pooyan-dadvand

Thanks!

Use PVS as geometry container

ab05c3c

rubenzorrilla added the Kratos Core label Nov 11, 2024

rubenzorrilla requested a review from pooyan-dadvand November 11, 2024 15:20

rubenzorrilla self-assigned this Nov 11, 2024

rubenzorrilla marked this pull request as ready for review November 11, 2024 18:11

rubenzorrilla requested a review from a team as a code owner November 11, 2024 18:11

sunethwarna reviewed Nov 12, 2024

View reviewed changes

pooyan-dadvand requested changes Nov 15, 2024

View reviewed changes

rubenzorrilla added 2 commits November 21, 2024 17:27

Merge branch 'master' into core/geometry-container-with-pvs

491cad1

@pooyan-dadvand suggestion

c97fc53

rubenzorrilla enabled auto-merge November 21, 2024 16:54

rubenzorrilla disabled auto-merge November 21, 2024 16:54

pooyan-dadvand approved these changes Nov 22, 2024

View reviewed changes

rubenzorrilla merged commit 04ac567 into master Nov 22, 2024
11 checks passed

rubenzorrilla deleted the core/geometry-container-with-pvs branch November 22, 2024 08:27

rubenzorrilla mentioned this pull request Nov 23, 2024

[Core] Geometry based connectivity preserve modeler #12794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Use PointerVectorSet as geometry container #12843

[Core] Use PointerVectorSet as geometry container #12843

rubenzorrilla commented Nov 11, 2024

rubenzorrilla commented Nov 11, 2024

sunethwarna commented Nov 11, 2024

sunethwarna commented Nov 11, 2024 •

edited

Loading

tteschemacher commented Nov 11, 2024

rubenzorrilla commented Nov 12, 2024 •

edited

Loading

sunethwarna commented Nov 12, 2024 •

edited

Loading

sunethwarna left a comment

rubenzorrilla commented Nov 12, 2024

pooyan-dadvand left a comment

pooyan-dadvand Nov 15, 2024

rubenzorrilla Nov 15, 2024

pooyan-dadvand Nov 18, 2024

rubenzorrilla Nov 21, 2024

pooyan-dadvand Nov 15, 2024

rubenzorrilla Nov 21, 2024

pooyan-dadvand Nov 15, 2024

rubenzorrilla Nov 21, 2024

pooyan-dadvand Nov 15, 2024

rubenzorrilla Nov 21, 2024

pooyan-dadvand left a comment

[Core] Use PointerVectorSet as geometry container #12843

[Core] Use PointerVectorSet as geometry container #12843

Conversation

rubenzorrilla commented Nov 11, 2024

rubenzorrilla commented Nov 11, 2024

sunethwarna commented Nov 11, 2024

sunethwarna commented Nov 11, 2024 • edited Loading

tteschemacher commented Nov 11, 2024

rubenzorrilla commented Nov 12, 2024 • edited Loading

sunethwarna commented Nov 12, 2024 • edited Loading

sunethwarna left a comment

Choose a reason for hiding this comment

rubenzorrilla commented Nov 12, 2024

pooyan-dadvand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooyan-dadvand left a comment

Choose a reason for hiding this comment

sunethwarna commented Nov 11, 2024 •

edited

Loading

rubenzorrilla commented Nov 12, 2024 •

edited

Loading

sunethwarna commented Nov 12, 2024 •

edited

Loading