Skip to content

Correct handling of overlapping NNC and EDITNNC#6875

Draft
daavid00 wants to merge 1 commit intoOPM:masterfrom
daavid00:nncs
Draft

Correct handling of overlapping NNC and EDITNNC#6875
daavid00 wants to merge 1 commit intoOPM:masterfrom
daavid00:nncs

Conversation

@daavid00
Copy link
Member

Avoid writing twice to the INIT the NNCTRANS.

Before merging, adding this deck to the regression tests:

OPM/opm-tests#1502

Depends on OPM/opm-common#5025

Closes #6865

@daavid00 daavid00 added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Feb 24, 2026
|| directVerticalNeighbors(*cartDims, cartesianToActive, cellIdx1, cellIdx2);
};

auto alreadyAssigned = [outputNnc = &this->outputNnc_]
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, PR updated.

@daavid00 daavid00 marked this pull request as draft February 25, 2026 06:14
@daavid00
Copy link
Member Author

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

@blattms
Copy link
Member

blattms commented Feb 25, 2026

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,

}

if (isNumAquConn(cc1, cc2) || ! isDirectNeighbours(cc1, cc2)) {
if (isNumAquConn(cc1, cc2) || (! isDirectNeighbours(cc1, cc2) && ! alreadyAssigned(cc1, cc2))) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@daavid00 daavid00 Feb 26, 2026

Choose a reason for hiding this comment

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

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 daavid00 force-pushed the nncs branch 2 times, most recently from 10e76ec to 44062e2 Compare March 5, 2026 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible BUG with NNC and EDITNNC

3 participants