Skip to content

Fixed Out of Bounds NNCs #894

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
wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −12
Open

Fixed Out of Bounds NNCs #894

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
Member

Choose a reason for hiding this comment

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

This (and all the changes below checking the thickness of active cells) seem suspicious as we should never create NNCs over active cells. Aren't these changes allowing this?

If you change the meaning of a variable then you should probably also change the name to reflect this.

Copy link
Member

Choose a reason for hiding this comment

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

The case this fixed was when all cells where pinched but pv>0 i.e. pv \approx 1e-14. It should not be any difference between c_thin && active and (pv < minpvv[c]) && active treatment.


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
Member

Choose a reason for hiding this comment

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

Please remove commented code.,

// 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]) {
kk = kk_iter;
continue;
}
//bottom cell not active, hence no nnc is created
if (!actnum.empty() && !actnum[c_below]) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be missing the pore volume check that was commented out above.

kk = kk_iter;
continue;
}
Comment on lines +236 to +244
Copy link
Member

Choose a reason for hiding this comment

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

I guess this the first part of the bug fix?

Copy link
Member

Choose a reason for hiding this comment

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

It make do the same in this branch as in the other so one do not iterate for ever.


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));
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) ){
Copy link
Member

Choose a reason for hiding this comment

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

I guess, this is the second part of the bug fix if we would skip the thickness check above?

Copy link
Member

Choose a reason for hiding this comment

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

This is to be conistent.

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)) &&
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) ) )
{
// 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
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

You have a draft pullrequest on the same. It include communication even if trans=0 in case one have other effects like thermal or mechanics over this.

addOverlapLayerNoZeroTrans(grid, index, *it, owner, cell_part, exportList, addCornerCells, layers-1, trans, level);
}
else {
Expand Down
16 changes: 14 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 @@ -204,8 +204,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

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 @@ -223,6 +225,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 @@ -416,12 +421,19 @@ namespace cpgrid
}
else {
// Make the grid.
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,
pinchActive_copy,
tolerance_unique_points,
edge_conformal);
}
Expand Down