Skip to content

Fixes issues for mult pv #916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hnil wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes issues for mult pv #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions opm/grid/MinpvProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ MinpvProcessor::process(const std::vector<double>& thickness,
bool c_active = actnum.empty() || actnum[c];
bool c_thin = (thickness[c] <= z_tolerance);
bool c_thin_inactive = !c_active && c_thin;
bool c_low_pv_active = pv[c] < minpvv[c] && c_active;
bool c_low_pv_active = (pv[c] < minpvv[c] && c_active) || (c_thin && c_active);
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and everything below) breaks the variable names reflecting the content. Should be fixed. Holds also for most of the occurrences below.

So basically the if below checks whether the pore volume is below the threshold or the cell is thin (no matter if it is active or inactive).

We should make this clear from the variable name and if statements.

Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about the added condition. Are you sure that this is as it should be? Why?

I guess we need some regression test cases with active cells to check whether they vanish and NNCs are created.


if (c_low_pv_active || c_thin_inactive) {
std::array<double, 8> cz = getCellZcorn(ii, jj, kk, zcorn);
Expand Down Expand Up @@ -166,7 +166,7 @@ MinpvProcessor::process(const std::vector<double>& thickness,
bool active = actnum.empty() || actnum[c_below];
bool thin = (thickness[c_below] <= z_tolerance);
bool thin_inactive = !active && thin;
bool low_pv_active = pv[c_below] < minpvv[c_below] && active;
bool low_pv_active = ((pv[c_below] < minpvv[c_below]) && active) || (thin && active);


while ( (thin_inactive || low_pv_active) && kk_iter < dims_[2] )
Expand Down Expand Up @@ -223,13 +223,26 @@ MinpvProcessor::process(const std::vector<double>& thickness,
active = actnum.empty() || actnum[c_below];
thin = (thickness[c_below] <= z_tolerance);
thin_inactive = (!actnum.empty() && !actnum[c_below]) && thin;
low_pv_active = pv[c_below] < minpvv[c_below] && active;
low_pv_active = ((pv[c_below] < minpvv[c_below]) && active) || (thin && active);
}

// create nnc if false or merge the cells if true
if (mergeMinPVCells && c_low_pv_active) {
if (mergeMinPVCells){// && c_low_pv_active) {
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit suspicious. Normally this is false by default, right?

// try to make a topological connected grid
// in Flow this is currently called only for edge_conformal grids
// however zcorn inbetween is not modified to make zcorn sorted
// Set lower k coordinates of cell below to upper cells's coordinates.
// i.e fill the void using the cell below
if (kk==0 || kk_iter == dims_[2]) {
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (kk==0 || kk_iter == dims_[2]) {
// no top or bottom cell, nothing to do.
if (kk==0 || kk_iter == dims_[2]) {

kk = kk_iter;
continue;
}
//bottom cell not active, hence no nnc is created
if (!actnum.empty() && !actnum[c_below]) {
kk = kk_iter;
continue;
}

std::array<double, 8> cz_below = getCellZcorn(ii, jj, kk_iter, zcorn);
for (int count = 0; count < 4; ++count) {
cz_below[count] = cz[count];
Expand Down Expand Up @@ -258,15 +271,15 @@ MinpvProcessor::process(const std::vector<double>& thickness,
auto above_active = actnum.empty() || actnum[c_above];
auto above_inactive = !actnum.empty() && !actnum[c_above];
auto above_thin = thickness[c_above] < z_tolerance;
auto above_small_pv = pv[c_above] < minpvv[c_above];
auto above_small_pv = (pv[c_above] < minpvv[c_above]) || above_thin;

if ((above_inactive && above_thin) || (above_active && above_small_pv
&& (!pinchNOGAP || above_thin) ) ) {
for (k_above = kk - 2; k_above > 0; --k_above) {
c_above = ii + dims_[0] * (jj + dims_[1] * (k_above));
above_active = actnum.empty() || actnum[c_above];
above_inactive = !actnum.empty() && !actnum[c_above];
auto above_significant_pv = pv[c_above] > minpvv[c_above];
auto above_significant_pv = !((pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance));
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the variable defined above?

Suggested change
auto above_significant_pv = !((pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance));
auto above_significant_pv = !above_small_pv;

auto above_broad = thickness[c_above] > z_tolerance;

// \todo if condition seems wrong and should be the negation of above?
Expand All @@ -291,25 +304,30 @@ MinpvProcessor::process(const std::vector<double>& thickness,
option4ALLZero = option4ALLZero || (!permz.empty() && permz[c_above] == 0.0) || multz(c_above) == 0.0;
nnc_allowed = nnc_allowed && (computeGap(cz_above, cz_below) < max_gap) && (!pinchOption4ALL || !option4ALLZero) ;

//bool
above_small_pv = (pv[c_above] < minpvv[c_above]) || (thickness[c_above] < z_tolerance);
bool below_small_pv = (pv[c_below] < minpvv[c_below]) || (thickness[c_below] < z_tolerance);
if ( nnc_allowed &&
(actnum.empty() || (actnum[c_above] && actnum[c_below])) &&
pv[c_above] > minpvv[c_above] && pv[c_below] > minpvv[c_below]) {
!(above_small_pv) && !(below_small_pv) ){
Comment on lines +307 to +312
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels hard to follow all these negations.

                        //bool
                        auto above_big_pv = (pv[c_above] >= minpvv[c_above]) && (thickness[c_above] >= z_tolerance);
                        bool below_big_pv = (pv[c_below] >= minpvv[c_below]) && (thickness[c_below] >= z_tolerance);
                        if ( nnc_allowed &&
                             (actnum.empty() || (actnum[c_above] && actnum[c_below])) &&
                             above_big_pv &&! below_bigl_pv ){

result.add_nnc(c_above, c_below);
}
kk = kk_iter;
}
}
else
{
if (kk < dims_[2] - 1 && (actnum.empty() || actnum[c]) && pv[c] > minpvv[c] &&
if (kk < dims_[2] - 1 && (actnum.empty() || actnum[c]) && !((pv[c] < minpvv[c]) || (thickness[c] < z_tolerance)) &&
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing this without negating is easier to read.

multz(c) != 0.0)
{
// Check whether there is a gap to the neighbor below whose thickness is less
// than MAX_GAP. In that case we need to create an NNC if there is a gap between the two cells.
int kk_below = kk + 1;
int c_below = ii + dims_[0] * (jj + dims_[1] * kk_below);

if ((actnum.empty() || actnum[c_below]) && pv[c_below] > minpvv[c_below])
if ( (actnum.empty() || actnum[c_below])
&&
!((pv[c_below] < minpvv[c_below]) || (thickness[c_below] < z_tolerance) ) )
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

{
// Check MAX_GAP threshold
std::array<double, 8> cz = getCellZcorn(ii, jj, kk, zcorn);
Expand Down
2 changes: 1 addition & 1 deletion opm/grid/common/GridPartitioning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ int addOverlapLayer([[maybe_unused]] const CpGrid& grid,
int index = ix.index(*it);
auto owner = cell_part[index];
exportProcs.insert(std::make_pair(owner, 0));
if ( trans ) {
if ( trans && false) {
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated and might be better in another PR.

addOverlapLayerNoZeroTrans(grid, index, *it, owner, cell_part, exportList, addCornerCells, layers-1, trans, level);
}
else {
Expand Down
23 changes: 21 additions & 2 deletions opm/grid/cpgrid/processEclipseFormat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,10 @@ namespace cpgrid
return transMult.getMultiplier(cartindex, ::Opm::FaceDir::ZPlus) *
transMult.getMultiplier(cartindex, ::Opm::FaceDir::ZMinus);
};
bool mergeMinPVCells = edge_conformal;// try to make geometrically connected grids
Copy link
Copy Markdown
Member

@blattms blattms Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes in this file seem unrelated. Maybe these should be in another PR?


minpv_result = mp.process(thickness, z_tolerance, ecl_grid.getPinchMaxEmptyGap(),
poreVolume, ecl_grid.getMinpvVector(), actnumData, false,
poreVolume, ecl_grid.getMinpvVector(), actnumData, edge_conformal,
zcornData.data(), nogap, pinchOptionALL,
permZ, multZ, tolerance_unique_points);
if (!minpv_result.nnc.empty()) {
Expand All @@ -211,6 +213,9 @@ namespace cpgrid

// Add PINCH NNCs.
std::vector<Opm::NNCdata> pinchedNNCs;
if(edge_conformal){
assert(minpv_result.nnc.size() == 0);
}

for (const auto& [cell1, cell2] : minpv_result.nnc) {
nnc_cells[PinchNNC].insert({cell1, cell2});
Expand Down Expand Up @@ -394,7 +399,21 @@ namespace cpgrid
processEclipseFormat(new_g, ecl_state, nnc_cells, true, turn_normals, pinchActive, tolerance_unique_points);
} else {
// Make the grid.
processEclipseFormat(g, ecl_state, nnc_cells, false, turn_normals, pinchActive, tolerance_unique_points);
auto pinchActive_copy = pinchActive;
if(edge_conformal){
// In edge conformal grids we need to set the pinchActive flag to true
pinchActive_copy = true;
assert(nnc_cells[PinchNNC].empty());
assert(nnc_cells[ExplicitNNC].empty());
}
this->processEclipseFormat(g,
ecl_state,
nnc_cells,
false,
turn_normals,
pinchActive_copy,
tolerance_unique_points,
edge_conformal);
}

return minpv_result.removed_cells;
Expand Down