Activate WELPI and WPIMULT for use in Pyaction

[lisajulia]

To activate the WELPI keyword for the use in a PyAction, I had to add a way of communicating the old well production indices from the ActionHandler (i.e. the Well Model) back to the Schedule. For this, I have added the function setWellPIMap to the Schedule object, setting the attribute m_wellPIMap, a std::unordered_map<std::string, double>, of the Schedule . This map is set from the Action Handler before pending PyActions are executed.

Then, to further simplify the code, I have removed this map from the arguments of the handleKeyword, applyAction and iterateScheduleSection (there it was called target_wellpi), since these functions can now use the m_wellPIMap.
Then, I have removed a call that was done before for each ActionX: This call fetched such a map for the matching wells of each ActionX, but now, since we have the m_wellPIMap, these calls are not needed anymore.

It would be interesting to have a benchmark for this, possibly time and memory wise.

For the reference manual: With this PR and OPM/opm-simulators#5467, the keywords WELPI and WPIMULT can be used in the "insert keywords"-function in Pyaction.

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[lisajulia]

This PR contains the WELPI kw (#4133) and also WPIMULT, same for other two PRs OPM/opm-simulators#5467 and OPM/opm-tests#1193.

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[blattms]

What I am wondering is the lifetime of the member of wellPIMap. Please elaborate a bit.

It is a member, Yet it seems to be used only as a temporary.

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[lisajulia]

@blattms: I've updated the description, also we can discuss if this really is the best approach before merging! Thanks!

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[blattms]

benchmark please

[blattms]

No realize that this benchmark will not work as it needs the other branches. I'll try with the correct repos, but I don't know whether the cases tested even use ACTIONX/PYACTION.

[blattms]

benchmark opm-simulators=5467 opm-tests=1193 please

[ytelses]

Benchmark result overview:

Test	Configuration	Relative
opm-git	OPM Benchmark: drogon - Threads: 1	0.997
opm-git	OPM Benchmark: drogon - Threads: 8	1.23
opm-git	OPM Benchmark: punqs3 - Threads: 1	1
opm-git	OPM Benchmark: punqs3 - Threads: 8	1.027
opm-git	OPM Benchmark: smeaheia - Threads: 1	0.994
opm-git	OPM Benchmark: smeaheia - Threads: 8	1.135
opm-git	OPM Benchmark: spe10_model_1 - Threads: 1	1.001
opm-git	OPM Benchmark: spe10_model_1 - Threads: 8	1.003
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 1	0.971
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 8	1.145
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 1	0.988
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 8	1.062
opm-git	OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1	0.984
opm-git	OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8	1.067

• Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2545

[lisajulia]

Benchmark result overview:
Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.997
opm-git OPM Benchmark: drogon - Threads: 8 1.23
opm-git OPM Benchmark: punqs3 - Threads: 1 1
opm-git OPM Benchmark: punqs3 - Threads: 8 1.027
opm-git OPM Benchmark: smeaheia - Threads: 1 0.994
opm-git OPM Benchmark: smeaheia - Threads: 8 1.135
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.001
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.003
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 0.971
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 1.145
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.988
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.062
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 0.984
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 1.067

* Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2545

Hm.. the main change performance-wise is that instead of fetching the well productions indices for each action per step, we fetch those now once per step (only if there are pending actions or pyactions, if there are no actions, we don't fetch anything at all).
Can this change really produce such high differences, like
OPM Benchmark: drogon - Threads: 8 : 1.23
OPM Benchmark: flow_mpi_extra - Threads: 1: 0.971 ?

[blattms]

Let's rerun. A serial run should not be affected by this.

[blattms]

benchmark opm-simulators=5467 opm-tests=1193 please

[lisajulia]

jenkins build this opm-simulators=5467 opm-tests=1193 please

[lisajulia]

@blattms: what is the further plan with this?

[lisajulia]

jenkins build this opm-simulators=5467 please

[lisajulia]

benchmark opm-simulators=5467 please

[lisajulia]

jenkins build this opm-simulators=5467 please

[lisajulia]

jenkins build this opm-simulators=5467 please

[lisajulia]

jenkins build this opm-simulators=5467 please

[lisajulia]

jenkins build this opm-simulators=5467 please

[bska]

I haven't looked at the details here, I'll do that later, but I would really prefer that we not add the proposed data member since the Schedule is supposed to contain information that's inferred from the model input only. Of course the WELPI keyword is rather strongly coupled to the dynamic simulator state since we need the current PI (or II) values in order to interpret the input data, but I still think we should aim to keep the input and the dynamically calculated information distinct. Is there really no other way of supplying the PI values than through this side channel?

[lisajulia]

I haven't looked at the details here, I'll do that later, but I would really prefer that we not add the proposed data member since the Schedule is supposed to contain information that's inferred from the model input only. Of course the WELPI keyword is rather strongly coupled to the dynamic simulator state since we need the current PI (or II) values in order to interpret the input data, but I still think we should aim to keep the input and the dynamically calculated information distinct. Is there really no other way of supplying the PI values than through this side channel?

Yes, I know this is not ideal, yet I need to communicate back the well production indices back to the Schedule.
For an ACTIONX, this is possible through passing such a map in the call applyAction (https://github.com/OPM/opm-simulators/pull/5467/files#diff-ebf1a517eb80b2cd88977997fa304cc0c6bd1750afdf14a8186ba2a763d1dedfR240), yet for a Pyaction there is no such call:

• A Pyaction is invoked by

  opm-common/opm/input/eclipse/Schedule/Schedule.cpp

  Line 1984 in 780ce15

  SimulatorUpdate Schedule::runPyAction(std::size_t reportStep, const Action::PyAction& pyaction, Action::State& action_state, EclipseState& ecl_state, SummaryState& summary_state) {

• The actual python code is executed by

  opm-common/opm/input/eclipse/Python/PyRunModule.cpp

  Line 85 in 780ce15

  bool PyRunModule::run(EclipseState& ecl_state, Schedule& sched, std::size_t report_step, SummaryState& st, const std::function<void(const std::string&, const std::vector<std::string>&)>& actionx_callback) {

• Without reintroducing something like the actionx_callback function it's not possible to access the simulator from the PyRunModule anymore.

Hence, my approach was to give a map containing all PI values to the Schedule before the Pyactions and ActionX's get executed, then also this map does not need to be passed around in all applyAction functions like before.

It might also be a (possibly wiser) option to move all the Pyaction/Python classes to opm-simulators and make them aware of the ActionHandler, yet I have not estimated other implications this would have, since it's a rather large change.

[lisajulia]

jenkins build this opm-simulators=5467 please

[lisajulia]

@bska: How do you suggest to move on with this? Thanks!

[lisajulia]

jenkins build this opm-simulators=5467 please

[bska]

Hence, my approach was to give a map containing all PI values to the Schedule before the Pyactions and ActionX's get executed

As a side effect, this makes every action more expensive since this means unconditionally computing information that's needed only if the action involves WELPI.

then also this map does not need to be passed around in all applyAction functions like before.

To be honest, I actually like the separate argument. It makes the separation between static and dynamic information more explicit. By putting the current PIs into the Schedule object, it becomes an implicit argument to every member function of Schedule, but it's only needed for one of them (Schedule::handleKeyword()–admittedly a rather important member function) and, moreover, whether or not it contains consistent information is completely controlled outside of Schedule.

How do you suggest to move on with this

How does the current PyAction system work? Most of the examples in opm-tests/pyaction appear to go through one of the insert_keywords() overloads which in turn end up in Schedule::applyKeywords(). Is that the expected mechanism? If so, is there anything that prevents adding a kind of dynamic state parameter to Schedule::applyKeywords() and passing the PI information through that? Every invocation of PyRunModule::run() maintains a set of attributes in the opm_embedded module, and the current_report_step is updated on every call. Could you not pass additional state through such an attribute?

[lisajulia]

Closing as the alternative was merged