Correct handling of overlapping NNC and EDITNNC by daavid00 · Pull Request #6875 · OPM/opm-simulators

daavid00 · 2026-02-24T12:01:56Z

Avoid writing twice to the INIT the NNCTRANS.

Before merging, adding this deck to the regression tests:

akva2 · 2026-02-24T12:21:28Z

            || directVerticalNeighbors(*cartDims, cartesianToActive, cellIdx1, cellIdx2);
    };

+    auto alreadyAssigned = [outputNnc = &this->outputNnc_]


why capture as pointer? capture as ref, ie, [&outputNnc = this->outputNnc_]

Thanks, PR updated.

daavid00 · 2026-02-25T06:15:14Z

In draft mode until how to handle this is decided in #6865

blattms · 2026-02-25T10:31:08Z

I don't think this correct if the value in EDITNNC is not 0.

Because we apply and internalize EDITNNC for an input NNC in OPM/opm-common#5025, we will reapply the value in EDITNNC in the EDIT section another time in Transmissibility::applyEditNncToGridTrans. Note that there we apply one factor that incorporates all EDITNNC internalized from the GRID/EDIT section and specified in the SCHEDULE section.

I don't have the complete picture, yet, but maybe we should not apply EDITNNC to input NNCs in opm-common and always internalize. Then all NNCs are treated equal. Not sure whether we need to do something addtional for output, though,

blattms · 2026-02-25T10:49:11Z

            }

-            if (isNumAquConn(cc1, cc2) || ! isDirectNeighbours(cc1, cc2)) {
+            if (isNumAquConn(cc1, cc2) || (! isDirectNeighbours(cc1, cc2) && ! alreadyAssigned(cc1, cc2))) {


I am not sure what this change does and whether it is correct.

For each cell pair in (at least) CpGrid there is only one intersection, even if there is an explicitly specified NNC and one that is created because of the grid topology. (Because of this and the additional output of those NNCs, we substract the transmissibility of the explicitly specified NNC below).

If I understand this change correctly. then `alreadyAssigned returns true if there was an NNC in the deck for this cell pair. Doesn't this mean that the transmissibility of an NNC created by the topology of the grid is not output at all if there was also an explicit NNC in the deck?

In master using the test case in OPM/opm-tests#1502 (i.e., internally the generated NNC has a value of 0.10000000E+01) with:

NNC 1 1 1 2 1 2 6666, EDITNNCR 1 1 1 2 1 2 9999 -> TRANNNC 0.99990000E+04 0.33330000E+04 NNC 1 1 1 2 1 2 6666, EDITNNC 1 1 1 2 1 2 10 -> TRANNNC 0.66661000E+05 0.10000000E+01

Here in EclGenericWriter_impl.hpp the total transmissibility is assigned, and with this PR we avoid writing duplicated values, i.e., with these PRs:

NNC 1 1 1 2 1 2 6666, EDITNNCR 1 1 1 2 1 2 9999 -> TRANNNC 0.99990000E+04 NNC 1 1 1 2 1 2 6666, EDITNNC 1 1 1 2 1 2 10 -> TRANNNC 0.66760000E+04

On a related note I still think EDITNNC/EDITNNCR should be applied only to internally generated NNCs and not the ones given in the deck using the keyword NNC. I would be happy to update the code in different PRs if that is the case.

daavid00 · 2026-03-11T16:05:53Z

Marking the PR as ready to review. Two decks are added to the tests for these OPM/opm-tests#1502 (planning on adding them to the regression tests prior merging).

blattms · 2026-03-17T17:32:30Z

jenkins build this please.

blattms

I am not finished (transmissibility is missing), but I need a break.

The logic during output is now really complicated and has a lot of (linear) searches in it. This might degrade performance quite a bit and we might need to find a better way.

blattms · 2026-03-17T17:06:46Z

+                    if (level == 0) {
+                        auto candidate = std::lower_bound(nncData.begin(), nncData.end(),
+                                                          NNCdata { originCartIdxIn, originCartIdxOut, 0.0 });
+                        const auto transMlt = transMult.getRegionMultiplierNNC(originCartIdxIn, originCartIdxOut);


This might be expensive please only do it if we found a matching nnc.

Looking at other places where getRegionMultiplierNNC is called (e.g., here), then we also do not check for matching nncs, but instead this is done once inside the method.

blattms · 2026-03-17T17:23:21Z

+                                }
+                            }
+                            if (foundNncEditr) {
+                                break;


I don't understand this break. Where is the connection to editNNC below, that is skipped.
If this is needed then please add a comment.

Added a comment

blattms · 2026-03-17T17:41:14Z

+                            break;
+                        }
+                    }
+                }


I think we should prevent this linear search. It would take a lot of time.

Do you see a way?

Not really, but this search happens only once before writing the INIT and looping for nncData, then after updating all binary search as you suggested, then having this linear search should be fine.

blattms · 2026-03-17T17:48:50Z

jenkins build this please

blattms · 2026-03-17T17:52:17Z

Sorry somehow github messed my comments made on multiple lines.

blattms · 2026-03-17T20:02:05Z

benchmark please

daavid00 · 2026-03-18T19:38:16Z

Thanks @blattms for your review, I have updated the PR and tried to answer your comments. Let me know if this need further changes.

blattms · 2026-03-19T07:41:49Z

jenkins build this please

blattms · 2026-03-19T11:40:29Z

I think we will need to investigate the currently failing regression tests once @daavid00 is back:

In addition we might want to add further regression tests before merging.

blattms

🙈 my fault. Should have added the opm-common companion PR when calling jenkins.

I am running jenkins another time in opm-common and will merge if this succeeds as expected.

daavid00 added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Feb 24, 2026

akva2 reviewed Feb 24, 2026

View reviewed changes

daavid00 force-pushed the nncs branch from 33b1c13 to a0f81c5 Compare February 24, 2026 12:47

daavid00 marked this pull request as draft February 25, 2026 06:14

blattms reviewed Feb 25, 2026

View reviewed changes

daavid00 force-pushed the nncs branch 6 times, most recently from 9d698d5 to b7e8718 Compare March 11, 2026 15:32

daavid00 mentioned this pull request Mar 11, 2026

Correct handling of overlaping NNC and EDITNNC OPM/opm-common#5025

Merged

daavid00 marked this pull request as ready for review March 11, 2026 16:05

daavid00 requested a review from blattms March 11, 2026 16:06

blattms requested changes Mar 17, 2026

View reviewed changes

blattms reviewed Mar 18, 2026

View reviewed changes

Comment thread opm/simulators/flow/Transmissibility_impl.hpp

blattms reviewed Mar 18, 2026

View reviewed changes

Comment thread opm/simulators/flow/EclGenericWriter_impl.hpp

Correct handling of overlaping NNC and EDITNNC

cd2cfd5

daavid00 force-pushed the nncs branch 2 times, most recently from 882b99f to c7aa24a Compare March 18, 2026 16:50

Addressing review for output overlapping nncs

66de203

daavid00 force-pushed the nncs branch from c7aa24a to 66de203 Compare March 18, 2026 19:17

blattms approved these changes Mar 26, 2026

View reviewed changes

blattms merged commit 5145ab5 into OPM:master Mar 27, 2026
1 of 2 checks passed

daavid00 deleted the nncs branch March 27, 2026 18:33

daavid00 mentioned this pull request Mar 27, 2026

Adding tests for overlapping NNCs (inputed and internally generated) #6968

Merged

Conversation

daavid00 commented Feb 24, 2026 • edited by blattms Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daavid00 commented Feb 25, 2026

Uh oh!

blattms commented Feb 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daavid00 Feb 26, 2026 • 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!

daavid00 commented Mar 11, 2026

Uh oh!

blattms commented Mar 17, 2026

Uh oh!

blattms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blattms commented Mar 17, 2026

Uh oh!

blattms commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blattms commented Mar 17, 2026

Uh oh!

Uh oh!

Uh oh!

daavid00 commented Mar 18, 2026

Uh oh!

blattms commented Mar 19, 2026

Uh oh!

blattms commented Mar 19, 2026

Uh oh!

blattms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daavid00 commented Feb 24, 2026 •

edited by blattms

Loading

daavid00 Feb 26, 2026 •

edited

Loading

blattms commented Mar 17, 2026 •

edited

Loading