Improve refinement and partially support adaptivity for CpGrid #723

aritorto · 2024-05-23T13:14:10Z

This PR

does not affect the update of the reference manual.
replaces Improve refinement and (partially) support adaptivity for CpGrid #721.
improves the refinement of CpGrid as well as partially support adaptivity, for general corner-point grids.
Marking cells with no neighboring connections for refinement is not supported yet.

Cells can be marked for refinement, without any assumption on the shape of the total cells to be refined. Previously, only a block of cells.

The adapted grid is the mix of coarse cells (not involved in refinement) and the refined entities.

blattms

Nice work!
I will try to do this review iteratively. This is the review of CpGridData.cpp

opm/grid/CpGrid.hpp

blattms · 2024-06-13T06:33:00Z

opm/grid/cpgrid/CpGridData.cpp

+    // Refined cells in parent cell: k*cells_per_dim[0]*cells_per_dim[1] + j*cells_per_dim[0] + i
+    std::array<int,3> ijk = {0,0,0};
+    ijk[0] = idxInParentCell % cells_per_dim[0];
+    idxInParentCell -= ijk[0]; // k*cells_per_dim[0]*cells_per_dim[1] + j*cells_per_dim[0]


Nitpick:
The subtractions don't hurt, but I think they are not needed.
Integer devision should make sure that (M+n)/N == M/N holds for n<N and M being a multiple of N.

Thanks! I skip this one for now, might come back once I'm done with the other comments

opm/grid/cpgrid/CpGridData.cpp

blattms

Next batch of comments. Still two files to comment on.

blattms · 2024-06-13T10:02:11Z

tests/cpgrid/adapt_cpgrid_test.cpp

+    {
+        const auto& elem =  Dune::cpgrid::Entity<0>(*(coarse_grid.chooseData()[startingGridIdx]), elemIdx, true);
+        coarse_grid.mark(1, elem);
+        coarse_grid.getMark(elem);


This line could be removed. The check of the return value is 2 lines below, anyway.

blattms · 2024-06-13T10:18:22Z

tests/cpgrid/adapt_cpgrid_test.cpp

+                auto equiv_cell_iter = blockRefinement_leaf.geomVector<3>().begin();
+                while (cell.center() != equiv_cell_iter->center()) {
+                    ++equiv_cell_iter;
+                }
+                CHECK_COORDINATES(cell.center(), equiv_cell_iter->center());


Suggested change

auto equiv_cell_iter = blockRefinement_leaf.geomVector<3>().begin();

while (cell.center() != equiv_cell_iter->center()) {

++equiv_cell_iter;

}

CHECK_COORDINATES(cell.center(), equiv_cell_iter->center());

auto equiv_cell_iter = std::find_if(blockRefinement_leaf.geomVector<3>().begin(),

blockRefinement_leaf.geomVector<3>().end(),

[center1 = cell.center](const auto& center1){

return center1 == equiv_cell_iter->center())

});

BOOST_CHECK( equiv_cell_iter != blockRefinement_leaf.geomVector<3>().end());

The last line was already checked in the loop.
We should probably allow the centers to differ a bit inside of the lambda and use some tolerance when comparing.

I had some trouble with that lambda, but I did add the check of the iterator not being at the end.

blattms · 2024-06-13T10:19:37Z

tests/cpgrid/adapt_cpgrid_test.cpp

+                CHECK_COORDINATES(cell.center(), equiv_cell_iter->center());
+                for(const auto& coord: cell.center())
+                    BOOST_TEST(std::isfinite(coord));
+                BOOST_CHECK_CLOSE(cell.volume(), equiv_cell_iter->volume(), 1e-6);


We could try to lower the tolerance here.

blattms · 2024-06-13T10:25:31Z

tests/cpgrid/adapt_cpgrid_test.cpp

+                    bool closedCenter =  (std::abs(element.geometry().center()[0] - equiv_element_iter->geometry().center()[0]) < 1e-12) &&
+                        (std::abs(element.geometry().center()[1] - equiv_element_iter->geometry().center()[1]) < 1e-12) &&
+                        (std::abs(element.geometry().center()[2] - equiv_element_iter->geometry().center()[2])< 1e-12);
+
+                    while (!closedCenter) {
+                        ++equiv_element_iter;
+                        closedCenter = (std::abs(element.geometry().center()[0] - equiv_element_iter->geometry().center()[0]) < 1e-12) &&
+                            (std::abs(element.geometry().center()[1] - equiv_element_iter->geometry().center()[1]) < 1e-12) &&
+                            (std::abs(element.geometry().center()[2] - equiv_element_iter->geometry().center()[2])< 1e-12);
+                    }


To make the bullet-proof we should check whether the equiv_element_iter is valid:
while (equiv_element_iter != equiv_grid_view.end<0>() && !closedCenter)

Maybe even use std::find_if from the STL instead like suggested above? The we only need to check the return value of it.

I'm adding the check that the iterator is not the end one (in many places). With the lambda-expression suggested, I'm having some issues. I postpone that change for now. Thanks!

blattms · 2024-06-13T11:10:53Z

tests/cpgrid/adapt_cpgrid_test.cpp

+                    referenceElem_entity_center[c] /= cells_per_dim[0]*cells_per_dim[1]*cells_per_dim[2];
+                    referenceElem_entity_center_it[c] /= cells_per_dim[0]*cells_per_dim[1]*cells_per_dim[2];
+                }
+                BOOST_CHECK_CLOSE(referenceElemOneParent_volume, 1, 1e-6);


We could try to tighten the tolerance a bit here. On the other hand the grid is large enough, so there is probably no change for now.

blattms · 2024-06-13T11:40:16Z

tests/cpgrid/grid_lgr_test.cpp

+        while (!closedCenter) {
+            ++equiv_element_iter;
+            closedCenter = (std::abs(element.geometry().center()[0] - equiv_element_iter->geometry().center()[0]) < 1e-12) &&
+                (std::abs(element.geometry().center()[1] - equiv_element_iter->geometry().center()[1]) < 1e-12) &&
+                (std::abs(element.geometry().center()[2] - equiv_element_iter->geometry().center()[2])< 1e-12);
+        }


Same comment here as for the other test file. Check whether equiv_element_iter is invalid or even better use an STL algorithm,

blattms · 2024-06-13T11:51:46Z

opm/grid/cpgrid/Entity.hpp

+    // To determine that, we track the level of the Entity and
+    // check if it was marked for refinement in the previos state
+    // of current_view_data_
+    return (isLeaf() && hasFather());


Indeed this is WIP but works for only refining once. For multiple refinement we need to do this differently

Actually the WIP is accidentally still there. From your comment, I'm assuming I should keep it. Maybe this is also related to my potentially wrong interpretation of preAdapt, adapt, postAdapt...

blattms · 2024-06-13T11:52:58Z

opm/grid/cpgrid/Entity.hpp

+bool Entity<codim>::mightVanish() const
+{
+    const auto refinementMark = pgrid_ -> getMark(*this);
+    return (refinementMark == 1);


I this tested?
Shouldn't this be `return refinementMark < 0?

This is probably again related to my interpretation of how the marking-elements process goes. I assume that when your stating grid is certain_grid with N cells, you marked some element on that grid, let's sat n1< ... < nr < N. Those marked element get mark equal to 1 when they will vanished and replaced by their refined children cells. Is this correct? When the mark is 0 or -1, it means "do nothing" or "coarsening (not supported yet, and in this case, this cell and "all its siblings cells" will vanish)".

I added a few comments in some of the places where mightVanish() get tested, in adapted_cpgrid_test.cpp

opm/grid/cpgrid/Entity.hpp

blattms

One file to go.

opm/grid/CpGrid.hpp

blattms · 2024-06-13T13:06:41Z

opm/grid/CpGrid.hpp

+                                        const std::shared_ptr<cpgrid::CpGridData>& elemLgr_ptr) const;
+
+        /// @brief Determine if a refined corner is located on the boundary of the single-cell-refinement, and does not coincide with
+        ///        a preApradt-existing corner.


Suggested change

/// a preApradt-existing corner.

/// a preAdapt-existing corner.

opm/grid/CpGrid.hpp

blattms

Here is the next portion of my review.
Last file (CpGrid.cpp) is the largest to review. This is the first part. I am at defineChildToParentAndIdxInParentCell

opm/grid/cpgrid/CpGrid.cpp

blattms · 2024-06-13T13:39:41Z

opm/grid/cpgrid/CpGrid.cpp

+    if(data_.size()>1) {
+        // Get the level where the element was born and its index in that level.
+        const auto& elemLevel =  element.level();
+        const auto& elemInLevel = (elemLevel == 0) ? element.getOrigin() : element.getLevelElem();
+        // Mark element in its level
+        data_[elemLevel] -> mark(refCount, elemInLevel);
+    }
+    // Mark element (also) in current_view_data_
+    return current_view_data_-> mark(refCount, element);


Can't we make sure that CpGridData::mark is written such that it allows us to write (please skip if we can't):

Suggested change

if(data_.size()>1) {

// Get the level where the element was born and its index in that level.

const auto& elemLevel = element.level();

const auto& elemInLevel = (elemLevel == 0) ? element.getOrigin() : element.getLevelElem();

// Mark element in its level

data_[elemLevel] -> mark(refCount, elemInLevel);

}

// Mark element (also) in current_view_data_

return current_view_data_-> mark(refCount, element);

const auto& elemLevel = element.level();

return data_[elemLevel] -> mark(refCount, elemInLevel);

Note that I removed marking on the current_view_data_. Do we need that? If that is the case then please keep it.

I skip this comment until we meet and define how to design the adapt process ( in which grid we are allowed to mark elements, etc)

blattms · 2024-06-13T13:50:38Z

opm/grid/cpgrid/CpGrid.cpp

+
+bool CpGrid::adapt()
+{
+    const std::vector<std::array<int,3>>& cells_per_dim_vec = {{3,3,3}}; // Arbitrary chosen values.


I think {2, 2, 2} would be more intuitive, at least for me. Might not be the case for others.

Let's trust in your intuition then :)

blattms · 2024-06-13T13:58:03Z

opm/grid/cpgrid/CpGrid.cpp

+    const auto& preAdaptMaxLevel = this ->maxLevel();
+    for (int elemIdx = 0; elemIdx < static_cast<int>(current_view_data_->size(0)); ++elemIdx) {
+        const auto& element = cpgrid::Entity<0>(*current_view_data_, elemIdx, true);
+        assignRefinedLevel[elemIdx] = (this->getMark(element) == 1) ? (preAdaptMaxLevel +1) : 0;


Maybe more future proof for the case of multiple levels.

Suggested change

assignRefinedLevel[elemIdx] = (this->getMark(element) == 1) ? (preAdaptMaxLevel +1) : 0;

assignRefinedLevel[elemIdx] = (this->getMark(element) == 1) ? (element.level() + 1) : element.level();

Note that the purpose of assignRefinedLevel is not 100% clear to me. So I could very well be wrong

Clearer now. It is the level where the kids will be.

Great! I'll add this in the list of things to be discussed, just in case

opm/grid/cpgrid/CpGrid.cpp

tests/cpgrid/adapt_cpgrid_test.cpp

blattms

Next batch of comments.

I am still in CpGrid.cpp. Now function CpGrid::identifyRefinedCornersPerLevel

opm/grid/cpgrid/CpGrid.cpp

opm/grid/cpgrid/CpGridData.cpp

opm/grid/cpgrid/CpGrid.cpp

blattms

Next batch.

I still want to check the functions in line 3440 and below (CpGrid::isRefinedCornerInInteriorLgr, etc.).

opm/grid/cpgrid/CpGrid.cpp

opm/grid/cpgrid/Entity.hpp

aritorto · 2024-06-25T07:20:05Z

@blattms many thanks for your valuable feedback! I believe I handled all/most of your comments (being the main: (1) change count-argument with find-approach, and (2) solve the dangling pointers issue via swap).

I would say this PR is ready for a "final review"

aritorto · 2024-06-25T07:20:22Z

jenkins build this please

opm/grid/cpgrid/CpGrid.cpp

opm/grid/cpgrid/EntityRep.hpp

opm/grid/cpgrid/OrientedEntityTable.hpp

aritorto · 2024-06-26T13:24:30Z

@blattms Changes are pushed now! Many thanks again!

aritorto · 2024-06-26T13:24:39Z

jenkins build this please

blattms

Thanks a lot for the updates.

aritorto · 2024-06-26T15:44:53Z

jenkins build this please

aritorto mentioned this pull request May 23, 2024

Improve refinement and (partially) support adaptivity for CpGrid #721

Closed

aritorto force-pushed the refactorRefine branch from acd4339 to 5f7a8b1 Compare May 29, 2024 14:04

aritorto force-pushed the refactorRefine branch from f5fbb14 to 3b4cc68 Compare June 13, 2024 09:52

blattms reviewed Jun 13, 2024

View reviewed changes

blattms reviewed Jun 14, 2024

View reviewed changes

aritorto commented Jun 14, 2024

View reviewed changes

tests/cpgrid/adapt_cpgrid_test.cpp Show resolved Hide resolved

aritorto commented Jun 14, 2024

View reviewed changes

tests/cpgrid/adapt_cpgrid_test.cpp Show resolved Hide resolved

aritorto commented Jun 14, 2024

View reviewed changes

tests/cpgrid/adapt_cpgrid_test.cpp Show resolved Hide resolved

aritorto mentioned this pull request Jun 18, 2024

Refinement of a grid containing coarse and refined cells partially supported #727

Closed

blattms reviewed Jun 19, 2024

View reviewed changes

aritorto commented Jun 20, 2024

View reviewed changes

opm/grid/cpgrid/Entity.hpp Show resolved Hide resolved

aritorto force-pushed the refactorRefine branch from ed09ec1 to 25327dc Compare June 20, 2024 13:22

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/CpGrid.cpp Outdated Show resolved Hide resolved

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/CpGrid.cpp Outdated Show resolved Hide resolved

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/CpGrid.cpp Outdated Show resolved Hide resolved

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/CpGrid.cpp Outdated Show resolved Hide resolved

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/EntityRep.hpp Outdated Show resolved Hide resolved

blattms reviewed Jun 26, 2024

View reviewed changes

opm/grid/cpgrid/OrientedEntityTable.hpp Outdated Show resolved Hide resolved

blattms approved these changes Jun 26, 2024

View reviewed changes

Improve refinement and (partially) support adaptivity

5ffaed4

aritorto force-pushed the refactorRefine branch from eb4165c to 5ffaed4 Compare June 26, 2024 15:43

aritorto mentioned this pull request Jun 27, 2024

Add test to illustrate improvement in CARFIN keyword OPM/opm-tests#1189

Merged

blattms merged commit 3912465 into OPM:master Jun 27, 2024
1 check passed

aritorto mentioned this pull request Jun 27, 2024

Refine (multiple times) a CpGrid with coarse and refine cells #731

Merged

-                auto equiv_cell_iter = blockRefinement_leaf.geomVector<3>().begin();
-                while (cell.center() != equiv_cell_iter->center()) {
-                    ++equiv_cell_iter;
-                }
-                CHECK_COORDINATES(cell.center(), equiv_cell_iter->center());
+                auto equiv_cell_iter = std::find_if(blockRefinement_leaf.geomVector<3>().begin(),
+                                                                           blockRefinement_leaf.geomVector<3>().end(),
+                                                                           [center1 = cell.center](const auto& center1){
+                                                                               return center1 == equiv_cell_iter->center())
+                                                                           });
+                BOOST_CHECK( equiv_cell_iter != blockRefinement_leaf.geomVector<3>().end());

	/// a preApradt-existing corner.
	/// a preAdapt-existing corner.

	assignRefinedLevel[elemIdx] = (this->getMark(element) == 1) ? (preAdaptMaxLevel +1) : 0;
	assignRefinedLevel[elemIdx] = (this->getMark(element) == 1) ? (element.level() + 1) : element.level();

Improve refinement and partially support adaptivity for CpGrid #723

Improve refinement and partially support adaptivity for CpGrid #723

Conversation

aritorto commented May 23, 2024 • edited Loading

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

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

aritorto Jun 14, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

aritorto Jun 14, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

blattms left a comment • edited Loading

Choose a reason for hiding this comment

aritorto commented Jun 25, 2024

aritorto commented Jun 25, 2024

aritorto commented Jun 26, 2024

aritorto commented Jun 26, 2024

blattms left a comment

Choose a reason for hiding this comment

aritorto commented Jun 26, 2024

aritorto commented May 23, 2024 •

edited

Loading

aritorto Jun 14, 2024 •

edited

Loading

aritorto Jun 14, 2024 •

edited

Loading

blattms left a comment •

edited

Loading