Fix GCONSUMP efficiency factor application

[hakonhagland]

The GCONSUMP efficiency factor calculation was incorrectly applying group efficiency factors to the groups themselves, rather than following the the pattern used elsewhere in the code base:

• Groups should NOT be affected by their own efficiency factors
• Parent groups apply child efficiency factors when accumulating contributions

Changes:

• Fix GroupState::update_gconsump() to implement correct efficiency pattern
• Add test suite with simple and complex hierarchy scenarios

[hakonhagland]

jenkins build this please

[hakonhagland]

The jenkins build failure:

...
Report step  1/12 at day 31/365, date = 01-Feb-2015
ERROR: Uncaught std::exception when running tasklet: Input IJK index (29, 29, 0) not part of grid with dimensions 30 x 30 x 9..

Starting time step 0, stepsize 18.312 days, at day 31/59, date = 01-Feb-2015
 Newton its= 7, linearizations= 8 (0.2sec), linear its= 10 (0.2sec)
Simulation aborted as program threw an unexpected exception: Failure in the TaskletRunner while writing output.
CUDA ERROR (code = 35, CUDA driver version is insufficient for CUDA runtime version) at /var/lib/jenkins/hypre/hypre/src/utilities/general.c:282

seems to be unrelated to this PR. @akva2 Any idea?

[akva2]

The real error is

ERROR: Uncaught std::exception when running tasklet: Input IJK index (29, 29, 0) not part of grid with dimensions 30 x 30 x 9.

which means k is std::size_t{-1}. I can only assume there's something funky with the input file you are using.

[hakonhagland]

I can only assume there's something funky with the input file you are using.

@akva2 I am able to reproduce an error (though not exactly the same error as in the jenkins build) for the same test case locally both for the PR branch and the master branch, so it seems it is not related to the PR.

[akva2]

yeah, i ran into it in another build. there is definitely an issue.

[akva2]

yeah, it contains out of bounds access errors. i've opened a PR to disable the test, #6739

[hakonhagland]

jenkins build this please

[vkip]

I'm probably missing something, but to me it seems this PR does the exact same thing as the originial code, only in a more inefficient (though admittedly more readable) manner. Could you provide a running example demonstrating the issue this PR intends to fix?

[hakonhagland]

Could you provide a running example demonstrating the issue this PR intends to fix?

@vkip The original code has a subtle bug: the parent_gefac parameter is only applied when retrieving from cache (lines 538-542 in the original code), but since m_gconsump_rates.clear() is called at line 527, the cache is always empty during recursion, so parent_gefac is never used.

Additionally, rates is passed by reference and accumulated all the way up, resulting in all groups storing the same values.

You can verify this by checking out the original code and running the test:

git checkout HEAD~1 -- opm/simulators/wells/GroupState.cpp
cd build && ninja test_gconsump && ctest -R gconsump --output-on-failure
# Result: 8 test failures showing efficiency factors not applied
git checkout HEAD -- opm/simulators/wells/GroupState.cpp  # restore fix

Regarding efficiency: The memoization cache in the original code actually provided no benefit during recursion. Since the group hierarchy is a tree (each group has exactly one parent), and the cache is cleared at the start of update_gconsump(), each group is visited exactly once and the cache lookup never succeeds. Both versions have O(N) complexity where N is the number of groups.

[vkip]

Thank you, I finally see the issue(s) now, and agree that there's no real effect of the intended optimization here (beyond obfuscating the code). Thanks a lot for identifying and fixing this!

[vkip]

Looks good to me, just a minor comment on the new .DATA files.

[vkip]

I understand it is not the intension, but with a couple of very minor changes (PVTO record 2, EQUIL item 1) the data file could actually also be run with the simulator, possibly extending the usefulness in other situations.

[hakonhagland]

Good idea, I have added a commit to address this.

[vkip]

Great, thanks 👍

[hakonhagland]

Thank for the review!

[vkip]

Same for this file.

[hakonhagland]

jenkins build this please