Improve refinement and (partially) support adaptivity for CpGrid #721
Conversation
|
jenkins build this please |
aritorto
force-pushed
the
markElement
branch
5 times, most recently
from
671c8ad to
37ffe51
Compare
aritorto
changed the title
aritorto
force-pushed
the
markElement
branch
2 times, most recently
from
f5e6709 to
c83b921
Compare
|
jenkins build this please |
There was a problem hiding this comment.
First preliminary comments. Did not even finish markAndAdapt_check().
Two comments on it. I am not 100% sure, but the level indices might depend on whether the coarse_grid was already refined or not. Seems like we are assuming the latter.
Somtime we call the method with the same grid as other_grid and coarse_grid (&other_grid==&coarse_grid is true). That is not a problem, but some checks cannot be wrong in this case.
| { | ||
| const auto& elem = Dune::cpgrid::Entity<0>(*(coarse_grid.chooseData()[0]), elemIdx, true); | ||
| coarse_grid.mark(1, elem); | ||
| coarse_grid.getMark(elem); |
There was a problem hiding this comment.
This seems superfluous as it is also in the check in the next line
There was a problem hiding this comment.
I see your point. I'll modify preAdapt() so that we do not mark cells in this way, but via calling preApadt().
| } | ||
| bool preAdapt = coarse_grid.preAdapt(); | ||
| const auto& data = coarse_grid.chooseData(); | ||
| if(preAdapt) { |
There was a problem hiding this comment.
I think we should check that preAdapt is what we would assume. I guess it should be true if markedCells has entries.
There was a problem hiding this comment.
I see your point in this comment and the one above. I guess it would be better when preAdapt() takes markedCells (a vector with the selected indices to be refined) as a function argument (instead of manually -for-loop- marking each cell).
Thanks for your first feedback!
| Dune::CpGrid& other_grid, | ||
| bool isBlockShape) | ||
| { | ||
| int refinedLevel = coarse_grid.chooseData().size(); // size before calling adapt |
There was a problem hiding this comment.
| int refinedLevel = coarse_grid.chooseData().size(); // size before calling adapt | |
| int oldNumLevels = coarse_grid.chooseData().size(); // size before calling adapt |
I am assuming that is is the index of the leaf grid view.
There was a problem hiding this comment.
You're right. This makes me think of the inconvenience of this variable name. As you said, it's just the index of the leaf grid view.
Example: we start marking a few cells from GLOBAL grid (level 0 and unique grid so far). Due to refinement, we get a "refined-grid-1" (level 1?), and then the corresponding "leaf grid view 1". When we select cells from "leaf grid view 1" to be refined, then we get another "refined-grid-2 (level 2?)" and the corresponding "leaf grid view 2". Therefore, with index
0 -> GLOBAL grid
1-> refined-grid-1 (level 1)
2 -> leaf-grid-view-1 (after the next refinement, it's just another grid, with coarse and refined cells)
3-> refined-grid-2 (level 2?, or level 1 respected to the grid the marked cells were taken from)
4-> leaf-grid-view-2
Now it's only a name in the test, but this comment helps me to re-consider some other variable names, in adapt() code. Thanks!
| if(preAdapt) { | ||
| coarse_grid.adapt(cells_per_dim); | ||
| coarse_grid.postAdapt(); | ||
| BOOST_CHECK(static_cast<int>(data.size()) == refinedLevel+2); |
There was a problem hiding this comment.
| BOOST_CHECK(static_cast<int>(data.size()) == refinedLevel+2); | |
| BOOST_CHECK(static_cast<int>(data.size()) == refinedLevel+(refinedLevel == 1)?2:1); |
If there is already more than one level, then I think that we would throw away the old leaf grid view, add a level and then create a new leaf grid view.
There was a problem hiding this comment.
Oh, I see. This is related to my previous comment, too. In that case, the selection of cells to be refined should be only allow in the GLOBAL grid?
tests/cpgrid/adapt_cpgrid_test.cpp
Outdated
| const std::array<int, 3> cells_per_dim = {2,2,2}; | ||
| std::vector<int> markedCells; | ||
| coarse_grid.createCartesian(grid_dim, cell_sizes); | ||
| markAndAdapt_check(coarse_grid, cells_per_dim, markedCells, coarse_grid, false); |
There was a problem hiding this comment.
what happens in this case? coarse_grid should not get another level. Should we check that?
I seems like we are comparing the grid to itself here. Maybe we should rather use a copy:
Dune::CpGrid coarse_grid, non_refined_grid;
....
coarse_grid.createCartesian(grid_dim, cell_sizes);
non_refined_grid.createCartesian(grid_dim, cell_sizes);
markAndAdapt_check(coarse_grid, cells_per_dim, markedCells, non_refined_grid, false);
tests/cpgrid/adapt_cpgrid_test.cpp
Outdated
| const std::string lgr_name = {"LGR1"}; | ||
| other_grid.addLgrsUpdateLeafView({cells_per_dim}, {startIJK}, {endIJK}, {lgr_name}); | ||
|
|
||
| markAndAdapt_check(coarse_grid, cells_per_dim, markedCells, other_grid, true); |
| coarse_grid.adapt(cells_per_dim); | ||
| coarse_grid.postAdapt(); | ||
| BOOST_CHECK(static_cast<int>(data.size()) == refinedLevel+2); | ||
| const auto& adapted_leaf = *data[refinedLevel+1]; |
There was a problem hiding this comment.
I think the index here depends on how many levels were there in the first place.
| const auto& preAdapt_view = coarse_grid.levelGridView(refinedLevel-1); | ||
| // Note: preAdapt grid in level "refinedLevel -1" | ||
| // refined grid in level "refinedLevel" | ||
| // adapted grid in level "refinedLevel +1" |
There was a problem hiding this comment.
Seems to depend on how many levels were there before refinement.
| const std::array<int, 3> startIJK = {2,0,0}; | ||
| const std::array<int, 3> endIJK = {4,1,1}; // -> marked elements 2 and 3 |
There was a problem hiding this comment.
Should we add a variant where the cells are not on the boundary?
|
Replaced by #723 |
This PR intends to improve 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.
To do: Include (and improve the existing) documentation.