Point ids for refined grids and addLgrsUpdateLeafGridView refactorization #802

aritorto · 2024-11-21T09:54:41Z

This PR introduces a structure to manage the point global IDs of all refined child cells per parent. This structure ensures the correct assignment of global point IDs in refined grids, gathering already-assigned IDs for interior cells and distributing them for overlap cells.

Current approach avoids duplicated point ids in the following setting:
Let's say a block of cells to be refined is distributed in P_{i_0}, ..., P_{i_n}, with n+1 < grid.comm().size().
If all cells to be refined are not "seen"(neither interior nor overlap) for every process P with P != P_{i_j}, j = 0, ..., n.

Points that may have multiple ids as shown and explained in #806.
Cells that only share corners (not faces) with interior cells of a process are not included in its overlap layer, therefore, some point may have multiple ids.

Keeping this potential multiple point ids in mind, this PR allows distributing Local Grid Refinements (LGRs) across different processes, subject to first distribute level zero grid, and after that add the LGRs. To be able to run the simulation, some changes to respect this order must be done in opm-simulators.

Improvement/replacement for #801

Not relevant for the Reference Manual

aritorto · 2024-11-21T12:38:18Z

jenkins build this serial please

akva2 · 2024-11-21T13:13:50Z

some new warnings, please address

aritorto · 2024-11-21T14:53:17Z

jenkins build this serial please

aritorto · 2024-11-22T10:42:27Z

jenkins build this serial please

blattms

Thanks a lot.
I have a few suggestions where you should probably change the code.

blattms · 2024-11-25T09:58:42Z

CMakeLists_files.cmake

@@ -202,6 +202,7 @@ list (APPEND PUBLIC_HEADER_FILES
  opm/grid/LookUpData.hh
  opm/grid/cpgrid/OrientedEntityTable.hpp
  opm/grid/cpgrid/ParentToChildrenCellGlobalIdHandle.hpp
+	opm/grid/cpgrid/ParentToChildCellToPointGlobalIdHandle.hpp


Please check whitespace here. Mabye you used a tab here? I would use copy and paste to get the correct whitespace.

blattms · 2024-11-25T10:00:19Z

opm/grid/CpGrid.hpp

@@ -1089,6 +1072,34 @@ namespace Dune
                                                   int elemLgr)  const;
        /// --------------- Auxiliary methods to support Adaptivity (end) ---------------

+              // @brief Check if there non neighboring connections on blocks of cells selected for refinement.


Please fix indentation. and use three slashes ///

opm/grid/CpGrid.hpp

opm/grid/cpgrid/CpGrid.cpp

blattms · 2024-11-25T10:49:35Z

opm/grid/cpgrid/CpGrid.cpp

+        getIJK(element.index(), ijk);
+        for (std::size_t level = 0; level < startIJK_vec.size(); ++level) {
+            bool belongsToLevel = true;
+            for (int c = 0; c < 3; ++c) {
+                belongsToLevel = belongsToLevel && ( (ijk[c] >= startIJK_vec[level][c]) && (ijk[c] < endIJK_vec[level][c]) );
+                if (!belongsToLevel)
+                    break;
+            }


Do we mark the cells of an LGR? If we do we can probably simplify this.

Code is slightly simplified in a coming-soon-commit. In nonNNCsSelectedCellsLGR(...) we only check if the selected cells for refinement (via blocks with startIJK endIJK values) have NNCs. If at least one cell of one block has NNCs, we return false.

We mark elements for refinement in CpGrid::markElemeAssignLevelDetectActiveLgrs(...)

blattms · 2024-11-25T15:44:07Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        // LGR1 dim 6x2x2 -> 7x3x3 = 63 points
+        std::vector<int> local_point_ids;
+        local_point_ids.reserve(63); // expected_point_ids in LGR1
+        for (const auto& element : elements(grid.levelGridView(1))) {


What is the reason why we do not iterate over vertices directly?

Good point!

blattms · 2024-11-25T15:45:45Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        }
+        auto [all_point_ids, displPoint ] = Opm::allGatherv(local_point_ids, grid.comm());
+        const std::set<int> all_point_ids_set(all_point_ids.begin(), all_point_ids.end());
+        // Difference with preveious test case:


Typo still there, though 😅 preveious -> previous

sorry for that, I thought I fixed it...

blattms · 2024-11-25T15:49:17Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        // - P1 does NOT see cells 2 and 6, but sees the rest of them.
+        // - P2 does NOT see cell 13, but sees all the others.
+        // - P3 does NOT see cell 1, 5 and 17, but sees all the others.
+        // Remark: the LGR is distributed only in 2 processes which are the only two processes seeing these cells.


I don't understand this sentence. Are there more than 1 LGR (which is said to be distributed among all processes above)

I will try to improve the comment. There is only one LGR here. The block of parent cells has dimension 2x2x2, the parent cell indices are {1,2,5,6,13,14,17,18}. Each parent cell gets refined into 2x2x2 children, therefore the LGR dimension is 4x4x4.

That comment is probably a copy-paste misunderstanding from the previous case. Sorry for that!

blattms · 2024-11-25T15:50:09Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        auto [all_point_ids, displPoint ] = Opm::allGatherv(local_point_ids, grid.comm());
+        const std::set<int> all_point_ids_set(all_point_ids.begin(), all_point_ids.end());
+        // Difference with preveious test case:
+        // Global cell ids of cells to be refined = {1,2,5,6,13,14,17,18}


Please indicate that this a bug originating from the way that we add overlap cells.

I am still not sure. Do we test how things should be below, or do we test in a certain way, just to make the test succeed for now (i.e. 145 is actually not the correct number). If it is the latter then the comment does not really tell me that.

All the comments are to explain why we get more than the expected amount of point ids. I could remove the check since in principle it's not what we want. On the other hand, there is this PR #806 showing that "we cannot do better" in some cases... Shall I remove all the "checks" of cases where we cannot guarantee unique point ids?

blattms · 2024-11-25T15:50:31Z

opm/grid/cpgrid/ParentToChildCellToPointGlobalIdHandle.hpp

+    // Not every cell has children. When they have children, the amount might vary.
+    bool fixedSize(std::size_t, std::size_t)
+    {
+        return false;
+    }
+    // Only communicate values attached to cells.
+    bool contains(std::size_t, std::size_t codim)
+    {
+        return codim == 0;
+    }
+    // Communicate variable size: 1 (rank) + (8* amount of child cells) from an interior parent cell from level zero grid.


These comments seem more like comments about the implementation. Maybe move them inside the methods?

aritorto · 2024-11-26T13:53:12Z

@blattms all commnets have been addressed, many thanks for your review!

aritorto · 2024-11-26T13:53:26Z

jenkins build this serial please

blattms

Probably just nitpicks about documentation.

blattms · 2024-11-27T14:02:51Z

opm/grid/CpGrid.hpp

+        /// @param [in] startIJK_vec    Vector of ijk values denoting the start of each block of cells selected for refinement.
+        /// @param [in] endIJK_vec      Vector of ijk values denoting the end of each block of cells selected for refinement.
+        ///
+        /// @return bool   True when all blocks of cells do not contain any NNCs (non-neighboring-connection).


I don't think you specify a type here. Please use /// @return True if, "when" should be probably be "if"

blattms · 2024-11-27T14:05:16Z

opm/grid/CpGrid.hpp

+        bool nonNNCsSelectedCellsLGR( const std::vector<std::array<int,3>>& startIJK_vec,
+                                      const std::vector<std::array<int,3>>& endIJK_vec) const;
+
+        /// @brief Given blocks of cells selected for refinement, mark selected elements and assign them their corresponding


Everything after @brief until an empty comment line will be used as the brief documentation. Maybe we should shorten this a bit and move the rest after an empty comment line.

opm/grid/cpgrid/CpGridData.hpp

blattms · 2024-11-27T14:22:53Z

opm/grid/cpgrid/CpGrid.cpp

+    // To decide which "candidate" point global id wins, the rank is stored. The smallest ranks wins,
+    // i.e., the other non-selected candidates get rewritten with the values from the smallest (winner) rank.
+    std::vector<std::vector<int>> level_winning_ranks(cells_per_dim_vec.size());
+
+    for (std::size_t level = 1; level < cells_per_dim_vec.size()+1; ++level) {
+
+        level_cell_to_point[level -1] = currentData()[level]->cell_to_point_;
+        // Set std::numeric_limits<int>::max() to make sure that, during communication, the rank of the interior cell
+        // wins (int between 0 and comm().size()).
+        level_winning_ranks[level-1].resize(currentData()[level]->size(3), std::numeric_limits<int>::max());
+
+        for (const auto& element : elements(levelGridView(level))) {
+            // For interior cells, rewrite the rank value - later used in "point global id competition".
+            if (element.partitionType() == InteriorEntity) {
+                for (const auto& corner : currentData()[level]->cell_to_point_[element.index()]){
+                    int rank = comm().rank();
+                    level_winning_ranks[level -1][corner] = rank;
+                }
+            }
+        }
+    }


I think it would be much cleaner if this is moved into the constructor of the handle. The container level_winning_ranks is not used here and is an implementation detail that should rather be hidden.

I see your point, but the Handle does not see any grid, so changing the for-loop to elemIdx with size of level_cell_to_point_ would be okay, but then to detect if the each element is interior we need an Entity<0>. To create an Entity, we need each refined level grid.
It can be done, but implies passing to the constructor of the handle all refined level grids... I would say that this implementation detail is already hidden in CpGrid::selectWinnerPointIds() defintion

blattms · 2024-11-27T14:26:49Z

opm/grid/cpgrid/ParentToChildCellToPointGlobalIdHandle.hpp

+
+    // Scatter global ids of child cells of a coarse overlap parent cell
+    template <class B, class T> // T = Entity<0>
+    void scatter(B& buffer, const T& element, std::size_t num_children) // check num_children


Actually, num_children is not the number of children. It is the number integers received. If there are no children it is 1 and otherwise it is 8 times the number of children + 1.

Hence maybe change the variable name to a simple size to prevent wrong assumptions.

blattms · 2024-11-27T14:38:23Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        }
+        auto [all_point_ids, displPoint ] = Opm::allGatherv(local_point_ids, grid.comm());
+        const std::set<int> all_point_ids_set(all_point_ids.begin(), all_point_ids.end());
+        // Difference with preveious test case:


Typo still there, though 😅 preveious -> previous

blattms · 2024-11-27T14:41:33Z

tests/cpgrid/addLgrsOnDistributedGrid_test.cpp

+        auto [all_point_ids, displPoint ] = Opm::allGatherv(local_point_ids, grid.comm());
+        const std::set<int> all_point_ids_set(all_point_ids.begin(), all_point_ids.end());
+        // Difference with preveious test case:
+        // Global cell ids of cells to be refined = {1,2,5,6,13,14,17,18}


I am still not sure. Do we test how things should be below, or do we test in a certain way, just to make the test succeed for now (i.e. 145 is actually not the correct number). If it is the latter then the comment does not really tell me that.

aritorto · 2024-11-27T16:15:28Z

jenkins build this serial please

blattms

Thanks. Fair enough. Merging

aritorto force-pushed the splitCode branch from 0d31525 to e58bb18 Compare November 21, 2024 12:32

aritorto changed the title ~~[refactor] Improve readability by splitting code on addLgrsUpdateLeafGridView~~ Point global ids for refined level grids and addLgrsUpdateLeafGridView refactorization Nov 21, 2024

aritorto mentioned this pull request Nov 21, 2024

Unique point global ids for refined level grids #801

Closed

aritorto requested a review from blattms November 21, 2024 12:47

aritorto changed the title ~~Point global ids for refined level grids and addLgrsUpdateLeafGridView refactorization~~ Point ids for refined grids and addLgrsUpdateLeafGridView refactorization Nov 21, 2024

aritorto mentioned this pull request Nov 25, 2024

Global refinement partially supported on distributed CpGrid #803

Closed

blattms requested changes Nov 25, 2024

View reviewed changes

This was referenced Nov 26, 2024

Overlap layer size 1 affects uniqueness of point global ids for CpGrid with LGRs #804

Open

On loadBalance(...) "addCornerCells" should be called "addEdgeCells"? #806

Closed

blattms reviewed Nov 27, 2024

View reviewed changes

aritorto force-pushed the splitCode branch from 39930d7 to dd953a8 Compare November 27, 2024 15:21

Point global ids for refined level grids

d36ced5

aritorto force-pushed the splitCode branch from 01c5337 to d36ced5 Compare November 27, 2024 16:15

blattms approved these changes Nov 27, 2024

View reviewed changes

blattms merged commit ec75718 into OPM:master Nov 27, 2024
1 check passed

aritorto deleted the splitCode branch December 4, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point ids for refined grids and addLgrsUpdateLeafGridView refactorization #802

Point ids for refined grids and addLgrsUpdateLeafGridView refactorization #802

aritorto commented Nov 21, 2024 •

edited

Loading

aritorto commented Nov 21, 2024

akva2 commented Nov 21, 2024

aritorto commented Nov 21, 2024

aritorto commented Nov 22, 2024

blattms left a comment

blattms Nov 25, 2024

blattms Nov 25, 2024

blattms Nov 25, 2024

aritorto Nov 26, 2024

blattms Nov 25, 2024

aritorto Nov 26, 2024

blattms Nov 25, 2024

blattms Nov 27, 2024 •

edited

Loading

aritorto Nov 27, 2024

blattms Nov 25, 2024

aritorto Nov 26, 2024 •

edited

Loading

blattms Nov 25, 2024

blattms Nov 27, 2024 •

edited

Loading

aritorto Nov 27, 2024

blattms Nov 25, 2024

aritorto commented Nov 26, 2024

aritorto commented Nov 26, 2024

blattms left a comment

blattms Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024

blattms Nov 27, 2024

aritorto Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024

blattms Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading

aritorto commented Nov 27, 2024

blattms left a comment

Point ids for refined grids and addLgrsUpdateLeafGridView refactorization #802

Point ids for refined grids and addLgrsUpdateLeafGridView refactorization #802

Conversation

aritorto commented Nov 21, 2024 • edited Loading

aritorto commented Nov 21, 2024

akva2 commented Nov 21, 2024

aritorto commented Nov 21, 2024

aritorto commented Nov 22, 2024

blattms 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

blattms Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aritorto Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blattms Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aritorto commented Nov 26, 2024

aritorto commented Nov 26, 2024

blattms left a comment

Choose a reason for hiding this comment

blattms Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aritorto Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blattms Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

blattms Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

aritorto commented Nov 27, 2024

blattms left a comment

Choose a reason for hiding this comment

aritorto commented Nov 21, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading

aritorto Nov 26, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading

aritorto Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading

blattms Nov 27, 2024 •

edited

Loading