Fixed Out of Bounds NNCs #894

bska · 2025-07-14T11:37:23Z

Still observer some race condition on strange files with temperature

bska · 2025-07-14T11:38:10Z

I'm creating this PR in draft mode because it depends on, and contains, the earlier PR #814. I will keep the PR in a draft state until such time as it is ready for review and merging.

blattms · 2025-07-16T09:41:06Z

Woud it make sense to have a separate PR with just the bugfix?

bska · 2025-07-16T09:57:58Z

Woud it make sense to have a separate PR with just the bugfix?

For what it's worth, this is the separate PR. I'd like to get #814 into master first.

bska · 2025-08-25T15:45:34Z

Woud it make sense to have a separate PR with just the bugfix?

For what it's worth, this is the separate PR. I'd like to get #814 into master first.

The earlier PR was merged into the master branch. I'm marking this PR as "ready for review" and I'm running a build check.

bska · 2025-08-25T15:45:47Z

jenkins build this please

blattms

I have the feeling that I have made some of these comments somewhere else before.

We need tests in test_minpvprocessor.cpp that test these chnages and also a regression test for the changes of the processing of thin cells.

Maybe I am missing something, but somehow the treatment of thin cells is changed by this PR and we might end up with NNCs over active thin cells. Is that what we want?

A more elaborate description explaining ALL changes would help.

blattms · 2025-09-04T15:06:57Z

opm/grid/MinpvProcessor.cpp

                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);


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.

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.

blattms · 2025-09-04T15:11:51Z

opm/grid/common/GridPartitioning.cpp

        auto owner = cell_part[index];
        exportProcs.insert(std::make_pair(owner, 0));
-        if ( trans ) {
+        if ( trans && false) {


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.

blattms · 2025-09-04T15:23:03Z

opm/grid/MinpvProcessor.cpp

+                        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;
+                        }


I guess this the first part of the bug fix?

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

blattms · 2025-09-04T15:30:28Z

opm/grid/MinpvProcessor.cpp

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


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

blattms · 2025-09-04T15:31:06Z

opm/grid/MinpvProcessor.cpp


                    // create nnc if false or merge the cells if true
-                    if (mergeMinPVCells && c_low_pv_active) {
+                    if (mergeMinPVCells){// && c_low_pv_active) {


Please remove commented code.,

blattms · 2025-09-04T15:32:18Z

opm/grid/MinpvProcessor.cpp

                        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) ){


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

This is to be conistent.

hnil · 2025-09-05T07:36:49Z

General comment: this fixes does two tings (if I remember correctly)

remove some issues in extreem cases where volume of a collapsed cells is calculated to > 0
remove infinite loops in the pinchActive false case

The part on mergeMinPV true is motivated by having a possibility to do pinchlike processing without needing to add artificial faces. This is need for doing mechanics. It probably will be highly favorable in other cases like thermal also.

There still a bug I do not know how to handle properly which is that the active cells between process_grdecl and minpvprocessor may be different due to the pv = 0 and c_thin may not be the same.

-- still observer some race condition on strange files with temperature

bska added the manual:bugfix This PR is a bug fix and should be noted in the manual label Jul 14, 2025

bska mentioned this pull request Jul 14, 2025

Expand Support for Geomechanical Workflows With Fracturing OPM/opm-simulators#6371

Draft

bska force-pushed the geomech-2 branch from c3d2820 to 829e3c9 Compare July 17, 2025 12:04

bska force-pushed the geomech-2 branch 2 times, most recently from 4ea01c4 to 1a63207 Compare August 15, 2025 15:45

bska force-pushed the geomech-2 branch 4 times, most recently from b1097e2 to 307f2dd Compare August 21, 2025 10:18

bska force-pushed the geomech-2 branch 4 times, most recently from 47bbe2b to 0e49135 Compare August 25, 2025 15:43

bska marked this pull request as ready for review August 25, 2025 15:45

bska force-pushed the geomech-2 branch 3 times, most recently from f0e5267 to d24419e Compare August 29, 2025 13:02

bska force-pushed the geomech-2 branch from d24419e to 04f8997 Compare September 4, 2025 13:19

blattms requested changes Sep 4, 2025

View reviewed changes

bska force-pushed the geomech-2 branch 3 times, most recently from 16d86d0 to 99db4f7 Compare September 12, 2025 08:36

bska force-pushed the geomech-2 branch 2 times, most recently from 274cde7 to 8dcfa1f Compare September 18, 2025 07:24

bska force-pushed the geomech-2 branch from 8dcfa1f to bc7addf Compare September 18, 2025 13:40

bska force-pushed the geomech-2 branch 2 times, most recently from 10b895e to f3263d7 Compare October 9, 2025 08:31

-- fixed out of bound nnc

5ec2afd

-- still observer some race condition on strange files with temperature

bska force-pushed the geomech-2 branch from f3263d7 to 5ec2afd Compare October 10, 2025 06:57

Fixed Out of Bounds NNCs #894

Are you sure you want to change the base?

Fixed Out of Bounds NNCs #894

Uh oh!

Conversation

bska commented Jul 14, 2025

Uh oh!

bska commented Jul 14, 2025

Uh oh!

blattms commented Jul 16, 2025

Uh oh!

bska commented Jul 16, 2025

Uh oh!

bska commented Aug 25, 2025

Uh oh!

bska commented Aug 25, 2025

Uh oh!

blattms left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnil commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

blattms left a comment •

edited

Loading

hnil commented Sep 5, 2025 •

edited

Loading